Add testing setting to force reverse primary keys#94017
Add testing setting to force reverse primary keys#94017amosbird wants to merge 22 commits intoClickHouse:masterfrom
Conversation
|
Workflow [PR], commit [dfa5a1f] Summary: ❌
AI ReviewSummaryThis PR introduces a testing setting Findings
ClickHouse Rules
Performance & Safety
Final Verdict
|
|
I saw a link to this on the roadmap, and GitHub claims this PR will close the roadmap ticket, maybe because of the word "resolves" in the reference to the comment? |
…lickHouse#94017 Add force_primary_key_reverse_order setting that forces all ORDER BY columns in CREATE TABLE to DESC for stress-testing the reverse key feature across the entire functional test suite. Auto-enables allow_experimental_reverse_key. Also fix style check by adding allow_experimental_reverse_key to the experimental settings ignore list.
8ca9673 to
4b4a044
Compare
…lickHouse#94017 Add force_primary_key_reverse_order setting that forces all ORDER BY columns in CREATE TABLE to DESC for stress-testing the reverse key feature across the entire functional test suite. Auto-enables allow_experimental_reverse_key. Also fix style check by adding allow_experimental_reverse_key to the experimental settings ignore list.
…lickHouse#94017 Add force_primary_key_reverse_order setting that forces all ORDER BY columns in CREATE TABLE to DESC for stress-testing the reverse key feature across the entire functional test suite. Auto-enables allow_experimental_reverse_key. Also fix style check by adding allow_experimental_reverse_key to the experimental settings ignore list.
| if (!as_table_saved.empty()) | ||
| create.is_create_empty = false; | ||
|
|
||
| if (!internal && mode <= LoadingStrictnessLevel::CREATE |
There was a problem hiding this comment.
force_primary_key_reverse_order introduces new CREATE TABLE behavior, but this PR only adds broad test exclusions/randomization changes and no focused positive test that verifies: (1) ORDER BY columns are rewritten to DESC, and (2) reversed keys still create successfully without explicitly setting allow_experimental_reverse_key. Please add a dedicated stateless test that asserts these two guarantees to prevent regressions.
| "input_format_parquet_use_native_reader_v3": lambda: min(1, random.randint(0, 5)), | ||
| "enable_lazy_columns_replication": lambda: random.randint(0, 1), | ||
| "allow_special_serialization_kinds_in_output_formats": lambda: random.randint(0, 1), | ||
| "force_primary_key_reverse_order": lambda: random.randint(0, 1), |
There was a problem hiding this comment.
Randomized setting at 50% may cause widespread test failures
Medium Severity
The force_primary_key_reverse_order setting is randomized with 50% probability (random.randint(0, 1)). Several tests that disable it explicitly create only Memory engine tables (e.g., 00578_merge_table_sampling.sql, 00632_aggregation_window_funnel.sql), suggesting the set of affected tests was over-estimated in some places and potentially under-estimated in others. With such aggressive randomization, any missed test that depends on physical MergeTree scan order without an explicit ORDER BY in its SELECT will fail intermittently, and there's no mechanism to detect these missed tests except CI flakes.
…lickHouse#94017 Add force_primary_key_reverse_order setting that forces all ORDER BY columns in CREATE TABLE to DESC for stress-testing the reverse key feature across the entire functional test suite. Auto-enables allow_experimental_reverse_key. Also fix style check by adding allow_experimental_reverse_key to the experimental settings ignore list.
1. ReadInOrderOptimizer (old analyzer path): matchSortDescriptionAndKey did not account for reverse_flags, causing wrong read direction when allow_experimental_analyzer = 0. 2. ALTER TABLE MODIFY ORDER BY: direction changes (ASC<->DESC) were not validated. Existing data parts are physically sorted in the original direction, so changing it silently corrupts sort order. Now throws BAD_ARGUMENTS. 3. MinMaxCount projection: used primary_key.reverse_flags which is always empty when PRIMARY KEY is explicitly specified (parsed without DESC support). Now uses sorting_key.reverse_flags instead.
…lickHouse#94017 Add force_primary_key_reverse_order setting that forces all ORDER BY columns in CREATE TABLE to DESC for stress-testing the reverse key feature across the entire functional test suite. Auto-enables allow_experimental_reverse_key. Also fix style check by adding allow_experimental_reverse_key to the experimental settings ignore list.
Add SET force_primary_key_reverse_order = 0 to tests that are incompatible with the forced DESC sorting key feature. This includes tests that: - Check specific ORDER BY / sorting key behavior - Verify specific data layout or merge behavior - Use features incompatible with reverse keys (e.g., specific projections) - Check SHOW CREATE TABLE output Also register force_primary_key_reverse_order in SettingsChangesHistory.cpp for version 26.3.
System log tables (query_log, part_log, etc.) are created with internal=true. Applying force_primary_key_reverse_order to them produces ORDER BY with DESC keywords that break CI's setup_log_cluster.sh sed-based ORDER BY modification, causing syntax errors like 'tuple(event_date DESC, event_time DESC)'. ATTACH queries load from existing metadata and must not be modified. Guard the applyForceReverseOrder call with !internal and mode <= LoadingStrictnessLevel::CREATE.
…m settings The setting was defaulting to true, which caused ~168 integration test failures since integration tests create MergeTree tables via normal CREATE TABLE (not internal). Changing the default to false and adding the setting to SettingsRandomizer enables random stress-testing in stateless tests without breaking integration tests. Tests that are incompatible with the setting already have explicit SET force_primary_key_reverse_order = 0.
The SET force_primary_key_reverse_order = 0 was placed inside a
Jinja2 list literal within the {% for %} block, breaking template
parsing. Moved it to the top of the file before any Jinja2 syntax.
Three root causes: - .sh tests: positional SQL args consumed by multitoken() Bool parser; fix by using --query flag instead of positional args - .sql tests with echoOn: inserted SET broke contiguous comment block for getTestTagsLength; fix by placing SET after all -- comments - query_log test: SET was logged; fix by filtering it from output
- 03733, 03533: Update reference files to include leaked comment lines (comments after SET break getTestTagsLength contiguity, echoOn echoes them) - 03006: Move SET before CREATE TABLE so tables aren't affected by random settings - 02572: Remove ilike filter, add SET entries to reference (SET is logged in query_log)
… tests Add SET force_primary_key_reverse_order = 0 (or CLICKHOUSE_CLIENT override for .sh tests) to 54 tests that fail when the setting is randomly enabled via SettingsRandomizer. Each test has a per-test comment explaining why the setting is incompatible. Also fix positional SQL args in 03276 and 03278 backup tests to work around the multitoken() Bool parsing bug in clickhouse-client.
Add SET force_primary_key_reverse_order = 0 (or CLICKHOUSE_CLIENT override for .sh tests) to tests that are incompatible with reverse key ordering. Each test has a one-line comment explaining the specific reason.
…ts and alphabetical order
EXPLAIN PIPELINE output depends on in-order aggregation optimization which is sensitive to MergeTree key direction.
- 02494_query_cache_totals_extremes: aggregation-in-order bug with reversed keys - 01656_sequence_next_node_long: sequenceNextNode depends on physical key order - 01231_distributed_aggregation_memory_efficient_mix_levels: distributed aggregation wrong counts - 02561_temporary_table_sessions: add CLICKHOUSE_URL override for curl-based queries - 03821_async_insert_same_param_names: async insert data cross-contamination - 02177_issue_31009: partial_merge join produces duplicates with reversed keys - 04004_alter_modify_column_ttl_without_type: SHOW CREATE TABLE direction sensitivity
…isks # Conflicts: # tests/queries/0_stateless/04004_integral_col_comparison_with_float_key_condition.sql
…tests Both tests rely on physical _block_number/_block_offset values from commit order projections, which change when the sorting key is reversed.
| if (!as_table_saved.empty()) | ||
| create.is_create_empty = false; | ||
|
|
||
| if (!internal && mode <= LoadingStrictnessLevel::CREATE |
There was a problem hiding this comment.
Applying applyForceReverseOrder here is too late for CREATE TABLE ... CLONE AS ....
The clone compatibility checks above compare source and target ordering before this rewrite. With force_primary_key_reverse_order=1, a clone can pass those checks and then have its ORDER BY flipped to DESC afterwards. CLONE AS reuses existing parts, so this can leave metadata saying reversed order while parts are still laid out in the original order, which can break read-order assumptions.
Please either skip this rewrite for create.is_clone_as, or move the rewrite earlier so clone compatibility is validated against the final ORDER BY.
LLVM Coverage Report
Changed lines: 90.21% (129/143) · Uncovered code |


Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
This PR adds a testing-only setting
force_primary_key_reverse_order. When enabled, allORDER BYcolumns inCREATE TABLEare automatically forced toDESC, regardless of the original definition, andallow_experimental_reverse_keyis enabled implicitly. This allows running the full functional test suite with reverse primary keys to stress-test correctness and stability without modifying individual tests. This resolves #93288 (comment)Documentation entry for user-facing changes
Note
Medium Risk
Although framed as a testing-only switch, this PR changes core MergeTree metadata/projection generation and ORDER BY read-in-order matching, which could affect query planning/results for tables with DESC sorting keys or during ALTERs.
Overview
Adds a new testing-only setting
force_primary_key_reverse_orderthat, duringCREATE TABLE, rewrites theORDER BYAST to force all key columns toDESCand implicitly enablesallow_experimental_reverse_key.Updates MergeTree internals to properly account for reversed sorting keys:
ReadInOrderOptimizernow matches required sort direction against the physical direction usingsorting_key.reverse_flags, minmax-count projection generation now uses the sorting key’s reverse flag (and its API is extended to acceptsorting_key), andALTER ... MODIFY ORDER BYnow rejects changing sort direction for existing key columns. Numerous functional tests are adjusted to explicitly disable the new setting where output/order assumptions would otherwise break.Written by Cursor Bugbot for commit f0828c0. This will update automatically on new commits. Configure here.