Conversation
clickhouse-gh
bot
added
the
pr-performance
|
Interesting, just enabling the setting is failing many fast tests |
We will have to allow mutations of indexed columns first. |
|
@alexey-milovidov yes, currently even explicitly created indexes have the same behaviour. cc @rschu1ze |
This comment was marked as resolved.
This comment was marked as resolved.
|
Dear @alexey-milovidov, this PR hasn't been updated for a while. Will you continue working on it? If not, please close it. Otherwise, ignore this message. |
|
Dear @alexey-milovidov, this PR hasn't been updated for a while. Will you continue working on it? If not, please close it. Otherwise, ignore this message. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Workflow [PR], commit [487a318] Summary: ❌
AI ReviewSummaryThis PR flips the default of Missing context
ClickHouse Rules
Final Verdict
|
|
Many issues in tests:
|
|
Note that there are a few places in the code that check whether |
|
Updating the PR after #91429, which fixes some of the issues found in the last CI run here |
Not too bad, not great either |
|
|
5 fast test left: 3 are new test that need adjusting, 2 seem like hitting the same bug. Then I need to tackle some other needed changes in private (tests and code) to make enabling this setting possible. In the meantime once this is green I'll randomize it in CI so I don't keep chasing new tests to adjust its |
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| Persists virtual column `_block_number` on merges. | ||
| )", 0) \ | ||
| DECLARE(Bool, add_minmax_index_for_numeric_columns, false, R"( | ||
| DECLARE(Bool, add_minmax_index_for_numeric_columns, true, R"( |
There was a problem hiding this comment.
This changes user-visible default behavior for MergeTree tables, but the PR does not add coverage for the new default path. Please add a stateless test that verifies a numeric column gets an auto min-max skipping index by default, and a complementary case with add_minmax_index_for_numeric_columns=0 to prove the opt-out still works.
src/Core/SettingsChangesHistory.cpp
Outdated
| }); | ||
| addSettingsChanges(merge_tree_settings_changes_history, "26.3", | ||
| { | ||
| {"add_minmax_index_for_numeric_columns", false, true, "Should give benefits."}, |
There was a problem hiding this comment.
26.3, but this default flip is being introduced now (current development cycle), not in the already released 26.3 line.
With the current placement, compatibility = 26.3 will not revert add_minmax_index_for_numeric_columns back to false (because the loop stops at version >= 26.3), so users requesting 26.3 behavior still get the new default.
Please move this entry to the version where the default actually changed (likely 26.4) so compatibility mode preserves old behavior correctly.
…add test Move the compatibility history entry from 26.3 to 26.4 so that `compatibility = 26.3` correctly reverts the setting to `false`. Add a stateless test verifying the new default (auto minmax index is created for numeric columns) and the opt-out path. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…led by default Add `add_minmax_index_for_numeric_columns = 0` to CREATE TABLE statements in tests that are not testing minmax index behavior, to prevent auto minmax indices from appearing in EXPLAIN output and changing granule counts or query plans. Affected tests: - 04029_multiply_monotonicity - 04003_deterministic_filter_chain_partition_pruning_key_condition_explain - 04003_text_index_search_mode - 03711_top_k_by_skip_index_negative - 04004_cast_nullable_monotonicity_key_condition - 03788_statistics_part_pruning - 03788_statistics_part_pruning_precision - 04032_alter_version_column_nonexistent - 03742_apply_row_policy_after_final Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…nmax_index_for_numeric_columns` The auto minmax index on the `value` column was appearing in EXPLAIN output, causing the test reference mismatch. Disable the setting in this test since it tests statistics-based pruning, not auto minmax indices. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
5 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
MergeTree tables will have
add_minmax_index_for_numeric_columnsby default. Resolves #70605Details
Resolves #70605