Workflow [PR], commit [64600d9]

Summary: ✅

AI Review

Summary

This PR enables use_top_k_dynamic_filtering and use_skip_indexes_for_top_k by default, adds compatibility history entries for these default flips, and hard-disables TopK optimization when FINAL is used to prevent incorrect deduplication behavior. I did not find any new correctness, safety, compatibility, or performance blockers in the current diff.

ClickHouse Rules

Final Verdict

• Status: ✅ Approve

I will check the failures.

@shankar-iyer FYI, it is #91782

@alexey-milovidov Let's enable only use_skip_indexes_for_top_k now? That aligns with our overall plan of skip indexes by default. use_top_k_dynamic_filtering is good when skip index is not present on the ORDER BY column, and provides only marginal improvement if skip index was present and used by use_skip_indexes_for_top_k.

use_top_k_dynamic_filtering is good when skip index is not present

I see it as a solid use-case. And indices by default will not always be enabled; there will be plenty of use cases without them. Why to afraid?

Why to afraid?

@alexey-milovidov

Best case -

SELECT URL, EventTime FROM hits WHERE URL LIKE '%google%' ORDER BY EventTime DESC LIMIT 10
  SETTINGS use_top_k_dynamic_filtering=0

10 rows in set. Elapsed: 3.374 sec. Processed 100.00 million rows, 10.02 GB (29.64 million rows/s., 2.97 GB/s.)


SELECT URL, EventTime FROM hits WHERE URL LIKE '%google%' ORDER BY EventTime DESC LIMIT 10
  SETTINGS use_top_k_dynamic_filtering=1

10 rows in set. Elapsed: 0.526 sec. Processed 100.00 million rows, 1.20 GB (189.97 million rows/s., 2.28 GB/s.)

But this one -

SELECT URL, EventTime FROM hits WHERE URL LIKE '%shankar%' ORDER BY EventTime DESC LIMIT 10
  SETTINGS use_top_k_dynamic_filtering=0

0 rows in set. Elapsed: 3.154 sec. Processed 100.00 million rows, 9.90 GB (31.70 million rows/s., 3.14 GB/s.)


SELECT URL, EventTime FROM hits WHERE URL LIKE '%shankar%' ORDER BY EventTime DESC LIMIT 10
  SETTINGS use_top_k_dynamic_filtering=0

0 rows in set. Elapsed: 3.507 sec. Processed 100.00 million rows, 10.30 GB (28.52 million rows/s., 2.94 GB/s.)

If there is not many rows to sort after filters, we have additional overhead of reading a 4-byte/8-byte column (Numeric/Date). What do you say - enable it and real world workloads should balance out?

All the test failures are not due to any issue in the optimization, but due to change in rows_read profile event, which I can address.

What do you say - enable it and real world workloads should balance out?

Yes, I think it will be beneficial on average!

Working on that in #91782

All committers have signed the CLA.

As #103553 is merged, let's update the branch here.

@groeneai, fix Logical error: AggregateFunctionArray and send a separate PR with the fix.

@alexey-milovidov — Logical error: AggregateFunctionArray on this PR is the chronic trunk bug STID 4870 family (parameters mismatch between Array wrapper and nested function for timeSeries*ToGridArray). It is not caused by this PR.

CIDB cross-PR (30 days, STID 4870 family): 43 master hits + 30+ unrelated PRs. The hit on this PR is STID 4870-4f21 (AST fuzzer (amd_debug) 2026-05-05 20:14 UTC), failing query:

SELECT timeSeriesResampleToGridWithStalenessArray(2, 120, toNullable(10), 10)(timestamps, `values`) FROM (...)

Error:

AggregateFunctionArray: parameters mismatch between Array wrapper 'timeSeriesResampleToGridWithStalenessArray' and nested function 'timeSeriesResampleToGridWithStaleness'.
Wrapper has 4 parameter(s): [UInt64_2, UInt64_120, UInt64_10, UInt64_10]
Nested function has 4 parameter(s): [Decimal64_'2', Decimal64_'120', Decimal64_'10', Decimal64_'10']

Root cause: AggregateFunctionTimeseriesBase normalizes its 4 timestamp parameters to Decimal64, but the Array combinator copies the user-typed UInt64 params. The chassert wrapper.params == nested.getParameters() was promoted to LOGICAL_ERROR in #102868, exposing the family.

Fix in flight: PR #103428 by @vitlibar — "Resubmit: Preserve parameter types in timeseries aggregate functions". OPEN + mergeable, awaiting review from @tavplubix since 2026-04-23.

My earlier PR #103079 (fixing it from the Array-combinator side: have the wrapper inherit the nested function's normalized params) was closed by @vitlibar on 2026-04-23 in favor of his approach (preserve original parameter types in TimeSeries instead of normalizing). Not filing a duplicate — would conflict with the active work.

Action: This failure on #99537 will resolve once #103428 merges. The bottleneck is review on #103428. Tracking under our internal P0 task since 2026-04-17.

Session: cron:clickhouse-ci-task-worker:20260506-024500

  Genuine regressions caused by the PR (changed, never seen on master in last 30d)                                                                                                                             
                                                                  
  ┌──────────┬───────────────────┬──────────────────────────────────────────────────┬─────────────────────────────┐                                                                                            
  │ Slowdown │       Test        │                      Query                       │ Master 30d (slower / total) │
  ├──────────┼───────────────────┼──────────────────────────────────────────────────┼─────────────────────────────┤                                                                                            
  │ 1.63x    │ string_sort #10   │ ORDER BY MobilePhoneModel LIMIT 300              │ 0 / 35 — new                │
  ├──────────┼───────────────────┼──────────────────────────────────────────────────┼─────────────────────────────┤                                                                                            
  │ 1.38x    │ sparse_column #52 │ test_sparse_1000 ORDER BY u8, u64 DESC LIMIT 100 │ 0 / 49 — new                │                                                                                            
  ├──────────┼───────────────────┼──────────────────────────────────────────────────┼─────────────────────────────┤                                                                                            
  │ 1.35x    │ sparse_column #47 │ test_full_1000 ORDER BY u8, u64 DESC LIMIT 100   │ 0 / 45 — new                │                                                                                            
  ├──────────┼───────────────────┼──────────────────────────────────────────────────┼─────────────────────────────┤                                                                                            
  │ 1.34x    │ sparse_column #49 │ test_full_10 ORDER BY u8, u64 DESC LIMIT 100     │ 0 / 58 — new                │
  ├──────────┼───────────────────┼──────────────────────────────────────────────────┼─────────────────────────────┤                                                                                            
  │ 1.20x    │ string_sort #117  │ ORDER BY MobilePhoneModel, SearchPhrase LIMIT 10 │ 0 / 30 — new                │
  ├──────────┼───────────────────┼──────────────────────────────────────────────────┼─────────────────────────────┤                                                                                            
  │ 1.16x    │ string_sort #108  │ ORDER BY SearchPhrase, URL LIMIT 10              │ 0 / 24 — new                │
  └──────────┴───────────────────┴──────────────────────────────────────────────────┴─────────────────────────────┘

Let me check.

Remove unrelated changes in vectorsearchstress.yml.

Update the branch to include the new changes that should fix the performance.

LLVM Coverage Report

Changed lines: 100.00% (27/27) · Uncovered code

Full report · Diff report