Workflow [PR], commit [deaeed3]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR adds predicate extraction for system.users.name equality and small IN filters so single-user lookups avoid scanning every configured user, while falling back for unsupported or overly large predicates. I did not find any remaining correctness, compatibility, or performance issue in the current PR head; the prior review concerns about filter propagation, duplicate names across access storages, nullable constants, and large duplicate-heavy IN lists are addressed in the current code.

Final Verdict

Status: ✅ Approve

Seems nice as you don't need to store more data for this optimization. Everything is already there.

Any blockers on merging this, @qoega ?

Thanks for pushing this @alexey-milovidov ! 🙏

@alexey-milovidov , fuzzer came out with:

`[src/Storages/System/StorageSystemUsers.cpp:64-68]` `extractNamesFromPredicateImpl` treats `and` as “union of supported children” instead of all-or-nothing extraction. 
For predicates like `name = 'alice' AND 0` (or contradictory conjuncts), the fast path
may still fetch users, and with strict limits like `max_rows_to_read`, 
this can raise an exception although full predicate evaluation would produce zero rows without reading user rows.

Suggested fix: make `AND` extraction strict; if any conjunct is not
a supported `name = const` / `name IN const-set` form, 
abort extraction and fall back to full scan (`std::nullopt`).

Do you concur, that we should be more strict about the optimization?

The review is on point. Let's first construct a test to demonstrate the incorrect behavior, then find a way to fix it.

Pushed 9ac59f182ee addressing the remaining clickhouse-gh thread on isNameNode:

• isNameNode now requires ActionsDAG::ActionType::INPUT in addition to result_name == "name", so the fast path only triggers on the genuine name column from getFilterSampleBlock.
• Added a regression query for a constant alias shadowing the column (SELECT 'alice' AS name FROM system.users WHERE name = 'alice').

Honest caveat: I couldn't reproduce an actual behavior divergence for that query against the pre-change code (the ALIAS-strip already lands on a constant whose result_name isn't name, so it fell back correctly). The change is a precision/robustness hardening with no impact on the legitimate fast path — verified by building both with and without it; the new query returns all users in both cases, and the existing stateless + integration tests still pass. CI was already green; branch trial-merges cleanly with master.

Addressed the Bugbot review comment about the unbounded IN fast path in 3bdfb9725f5.

The fast path looked up each requested name in every access storage, so a very large name IN (...) set (a huge literal list or a subquery result) could perform more lookups than a single full scan — the opposite of the intended contract. The fast path is now capped at a deliberate limit (max_names_for_fast_path = 1000); above the limit we fall back to the full findAll<User>() scan, which is exactly the pre-optimization behavior, so the fast path can never become slower than the full scan. Single-user lookups and short IN lists (the target case) are unchanged.

The limit value is a heuristic — there is no cheap way to count users without findAll, so a fixed bound is the pragmatic choice — and is easy to tune if you prefer a different number.

Extended 03914_system_users_predicate_pushdown.sh with a 1500-element IN set (above the limit) that must still return the correct result via the fallback. All cases pass locally.

@alexey-milovidov your own VirtualColumnUtils note is left as-is (it concluded the custom tryExtractNameFromPredicate is the right pattern, like StorageSystemZooKeeper::extractPathImpl).

Pushed 6f64502 addressing the two automated review findings:

1. Equality fast path was broken (the headline feature). The name = 'literal' path was guarded by value->column->size() != 1, but ActionsDAG::addColumn normalizes a constant node to a ColumnConst of size 0 (see the comment on ActionsDAG::Node::column), so the guard rejected every literal and extractNamesFromPredicateImpl returned nullopt — sending the lookup back to findAll<User>. The fast path never ran for equality. Fixed by reading the underlying value directly without a size guard, exactly like StorageSystemZooKeeper does for its path predicate.

2. IN fast path now bails before materializing a large set. As soon as the set exceeds max_names_for_fast_path it returns nullopt and falls back to the full scan, instead of copying every element first.

Test reworked to actually witness the fast path. The previous test could not catch issue (1): the output is identical whichever path runs, and its max_rows_to_read assertions are vacuous because that limit is not enforced for the single-chunk ReadFromSystemOneBlock source. It now runs against the server and checks system.query_log.read_rows — the fast path materializes only the matching users (verified locally: read_rows = 1 for equality, 2 for a 2-element IN, 0 for a missing user; the LIKE and large-IN fallbacks read all users). The integration test for duplicate names across storages also becomes meaningful now that the equality fast path actually runs.

The only red CI check is Stress test (arm_asan_ubsan), which fails to start the server with Too many marks in file ... skp_idx_idx.cmrk4 ... (CANNOT_READ_ALL_DATA) on a randomized-settings text index over an encrypted S3 cache. That is unrelated to system.users predicate pushdown.

Pushed f9e6039c404 addressing the two remaining automated review findings (the ⚠️ Request changes verdict), then merged the latest master.

1. NULL string constants no longer raise an exception. A Nullable(String) constant passes the type check, but name = CAST(NULL, 'Nullable(String)') would reach ColumnNullable::getDataAt, which throws on a NULL. The equality path now returns an empty candidate set when the constant isNullAt(0) (the fast path emits nothing, matching the full scan), and the IN loop skips NULL elements via isNullAt(i). ColumnConst::isNullAt forwards to the underlying value, so this is correct even though ActionsDAG normalizes the constant to a ColumnConst of size 0.

2. A large literal name IN (...) no longer materializes the right-hand side before falling back. The set size is now checked via the already-built set's getTotalRowCount() before calling buildOrderedSetInplace. For a literal tuple, FutureSetFromTuple fills its Set at construction, so the row count is available without copying anything; above max_names_for_fast_path we fall back to the full scan. This stops a large literal list from being duplicated/materialized just to reach the existing cap (the default use_index_for_in_with_subqueries_max_values = 0 does not bound it). The post-build elements->size() guard is kept for set kinds whose size is known only after building (e.g. subquery sets).

Tests. Extended 03914_system_users_predicate_pushdown with focused coverage for name = NULL and name IN (NULL, '<user>'), proving the fast path preserves full-scan semantics without raising an exception (correctness-only, so the assertions hold regardless of whether the fast path or the fallback runs).

The edited translation unit compiles cleanly; the change touches only stable Set/PreparedSets/Column APIs, and all pre-existing test outputs are preserved (the 1500-element large-IN case still falls back and returns the same result, now without materializing first).

The remaining red CI checks are unrelated: Stress test (arm_tsan) fails to start the server with Logical error: 'Unexpected exception in refresh scheduling' (STID 2508-3e7b, tracked by #106737, in BackgroundSchedulePool refresh scheduling, nothing to do with system.users), and the Performance Comparison (master_head) deltas are run-to-run noise (the cloud-benchmark verdict was no_change).

Addressed the automated review's remaining finding (the ⚠️ Request changes verdict on the large-IN cap) in deaeed3de3a.

The large-IN guard checked the deduplicated row count (Set::getTotalRowCount), but building the explicit set elements (buildOrderedSetInplace → Set::appendSetElements) filters the original right-hand side columns, so a duplicate-heavy or mostly-NULL list such as name IN ('x', 'x', ..., 'y') could stay under the limit by distinct count yet still force a scan of the whole right-hand side. FutureSetFromTuple now exposes getInputRowCount() (the original list length, O(1) from the construction-time deduplication filter), and the IN fast path bounds on it before buildOrderedSetInplace.

03914_system_users_predicate_pushdown.sh gains a duplicate-heavy IN case (asserting via read_rows that it falls back to the full scan) and a mostly-NULL IN case.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 97/116 (83.62%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report