Workflow [PR], commit [3c7c91d]

Summary: ❌

AI Review

Summary

This PR flips defaults for use_top_k_dynamic_filtering and use_skip_indexes_for_top_k to true, records the default change in SettingsChangesHistory under 26.4, and stabilizes an existing stateless test by explicitly disabling both settings. I did not find new correctness, safety, compatibility, or performance-risk issues in the current PR head. PR metadata is consistent with the code change and the changelog entry is user-readable and specific.

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.

LLVM Coverage Report

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

Full report · Diff report