Workflow [PR], commit [895480b]

Summary: ❌

AI Review

Summary

This PR fixes aggregate projection matching for multiple sumIf aggregates with different IN sets by adding a FutureSetFromTuple content hash and a planning setting to cap the comparison cost. The approach is mostly well covered, but the new hash is still not order-independent for some floating-point sets, so I would not approve yet.

Findings

⚠️ Majors

• [src/Interpreters/PreparedSets.cpp:202] getContentHash promises an order-independent hash for normalized IN set contents, but the sort order is based on column comparison while Set stores single numeric keys by their raw fixed-width key. For Float64, values such as 0.0 and -0.0 compare equal for sorting, so the stable permutation preserves input order; then updateHashFast hashes their distinct raw bytes. That means IN (0.0, -0.0) and IN (-0.0, 0.0) can produce different content hashes even though they contain the same Set keys, causing the projection matcher to miss a valid projection. Suggested fix: add a raw-byte or Set key-representation tie-breaker for compare-equal values, or hash rows from the same key representation that Set uses.

Tests

• ⚠️ Add a focused regression for the order-independent hash with reversed +0.0 and -0.0 values, or with distinct NaN payloads. The current tests cover strings, UInt64, duplicates, and repeated types, but not compare-equal/raw-distinct floating-point keys.

Final Verdict

Status: ⚠️ Request changes

Minimum required action: make FutureSetFromTuple::getContentHash order-independent for floating-point keys that compare equal but have distinct raw Set keys, and cover that case with a regression test.

Let's implement lazy calculation. And probably some limit on set size.

Hi @KochetovNicolai

What could be a reasonable default limit for a set size? I won't expect to see a performance degradation unless the set is absurdly large like in the order of 10^4 elements or more, but I could be obviously wrong.

Also, what do you think is expected to happen when the limit is violated? trigger an error? fall back?

This was fixed by #105146. Let's update the branch.

What could be a reasonable default limit for a set size? I won't expect to see a performance degradation unless the set is absurdly large like in the order of 10^4 elements or more, but I could be obviously wrong.

Agree, maybe 10^3 or 10^4 is ok.

Also, what do you think is expected to happen when the limit is violated? trigger an error? fall back?

I think in our case, we can say that every "big" set is not equal to any other set (we won't apply projection and so on)

I agree with AI comment and something like use_index_for_in_with_subqueries_max_values is still needed.

I agree with AI comment and something like use_index_for_in_with_subqueries_max_values is still needed.

The change alread uses use_index_for_in_with_subqueries_max_values

The AI comment was because computeContentHash was calling getKeyColumns, which calls fillSetElementsOnce and permanently materializes set->set_elements, bypassing the large-set guard in buildOrderedSetInplace.

I fixed that in 0521377

I've still added a separate setting. Looks like use_index_for_in_with_subqueries_max_values uses a bit differen semantic. Also, it's 0 by default (so no limit on set size), and I think is make sense to limit set compariosn by default.

Feel free to revert if you don't like my change.

Note:

Can we make FutureSetFromTuple::computeContentHash() implementation without sorting? Is there     
  position-independent hashing?                                                                     

● Yes — a set is unordered by definition, so you don't need to sort to hash it canonically. The
  standard trick is a commutative combination of per-row hashes:

  - Hash each row to a fixed-width value (e.g. SipHash128/CityHash128 over the serialized columns of
   that row).
  - Combine row hashes with a commutative op: addition mod 2^128 (i.e. __uint128_t accumulator).
  This is the textbook "AdHash" multiset hash — preserves multiplicity, is O(N), trivially
  parallelizable (partial sums per thread), and provably as collision-resistant as the underlying
  per-row hash. XOR works too but only as a set (not multiset) hash, because pairs of duplicates
  cancel — usually undesirable.

  Main tradeoff: the combiner is cheap, but the per-row hash dominates cost, so the win over
  sort+hash is only ~`log N. The real wins are (a) no extra memory for sorted copies, (b) 
  streaming/incremental update as rows are inserted, and (c) easy parallelization. If you also want 
  it position-independent across *column order*, you'd hash (column_name, value)` pairs and sum
  inside each row first.

But for this, we need to add something like IColumn::getPositionIndependentHash, so probably later.

PR / Stateless tests (arm_asan_ubsan, targeted) (pull_request) Failing after 55m

● Summary

  This is a CI infrastructure flake, not a real failure caused by the PR.

  What happened:
  - The job is Stateless tests (arm_asan_ubsan, targeted), running ~60 "auto-targeted" optimization/projection tests with --test-runs 50 --jobs 19 --flaky-check.
  - All 2837 attempted test runs passed (job.log:4609 says Failed: 0, Passed: 2837).
  - The wrapper killed the runner with SIGTERM after the ~47 min wall-clock timeout (Timeout exceeded [2817], job.log:4453).
  - The runner exited with -15, which is in ABORTED_RUN_EXIT_CODES, so functional_tests_results.py:238 synthesizes a "Server died" leaf for the report — that's misleading framing for a timeout kill.
  - The actual server process shut down cleanly afterwards: Background threads finished, grpc_shutdown done, Child process exited normally with code 0 (clickhouse-server.log). No <Fatal>, no sanitizer report, no stack trace in
  clickhouse-server.err.log.

  Why it timed out:
  The targeted selector grabbed every test whose name matches optimization/projection/plan (≈60 tests, several slow: 01655_plan_optimizations ~52s, 02317_distinct_in_order_optimization_explain ~60s, 03921_kafka_formats ~38s,
  03031_read_in_order_optimization_with_virtual_row*, ...). Multiplying by --test-runs 50 on ARM ASan+UBSan blew through the 47 min budget. Nothing in the diff makes those tests slower — the targeted selection itself is just too large for
   this config.

  Recommendation: rerun the job. Unrelated to your projection/sumIf change; safe to ignore for the PR's correctness.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 224/225 (99.56%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report

Found via ClickGap automated review. Please close or comment if this is incorrect or needs adjustment.

TL;DR

Fix aggregate projection not matched when multiple sumIf use different IN sets

Reproduction

CREATE TABLE t_sumif_proj (grp UInt8, col String, val UInt64, PROJECTION prj (SELECT grp, sumIf(val, col IN ('a','b')) AS m1, sumIf(val, col IN ('c','d')) AS m2 GROUP BY grp)) ENGINE = MergeTree ORDER BY grp; INSERT INTO t_sumif_proj SELECT number%4, ['a','b','c','d'][number%4+1], number FROM numbers(40); OPTIMIZE TABLE t_sumif_proj FINAL; SELECT grp, sumIf(val, col IN ('a','b')) AS m1, sumIf(val, col IN ('c','d')) AS m2 FROM t_sumif_proj GROUP BY grp ORDER BY grp SETTINGS optimize_use_projections=1, force_optimize_projection=1; SELECT grp, sumIf(val, col IN ('a','b')) AS m1, sumIf(val, col IN ('c','d')) AS m2 FROM t_sumif_proj GROUP BY grp ORDER BY grp SETTINGS optimize_use_projections=0;

Bisect

• Bisect skipped — bug predates available CI binaries. Maintainer-side bisect needed.

Affects

• ✓ Fix is on master — this PR's change fixes the bug on master. Branches below still have the bug and need the backport.
• Reproduces on: 26.5, 26.4, 26.3, 25.8
• Does NOT reproduce on: master
• Backport needed: 26.5, 26.4, 26.3, 25.8

Suggested next step

The fix has landed on master; the affected release branches above still need the backport.

Component: comp-query-optimizer
Severity: P2

cc fix PR author:
cc fix PR reviewer:

ClickGap · Finding: phase_d_pr104765