Support PREWHERE and trivial count for Memory tables - #116248
Support PREWHERE and trivial count for Memory tables#116248alexey-milovidov wants to merge 19 commits into
Conversation
PREWHERE (and the pushed-down row-level security filter) is applied inside MemorySource: only the columns of the conditions are read at first, and the remaining columns are read only for the blocks where some rows pass and only for the passing rows. For a table with SETTINGS compress = true a selective condition skips decompression of all other columns for the blocks it eliminates. StorageMemory::getColumnSizes reports real per-column in-memory sizes (compressed sizes when compress = true), which both enables the automatic WHERE -> PREWHERE move in the query plan optimization and lets it order conditions by the actual cost of reading their columns. SELECT count() FROM table is served from metadata (totalRows is exact, maintained under the write mutex). Motivated by benchmarking a compressed Memory table: ClickHouse/ClickBench#1590 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s and docs A materialized CTE and a GLOBAL subquery temporary table are filled during query execution, after the planner would have observed totalRows (as zero), so the trivial count optimization must not apply to them. Also update the in-code Memory engine documentation and add functional and performance tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ents Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Workflow [PR], commit [3e77002] Summary: ❌
AI ReviewSummaryThis PR adds Findings
Tests
Final Verdict
LLVM Coverage ReportMeasured on commit 3e77002.
Changed lines: Changed C/C++ lines covered: 315/328 (96.04%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-performance
|
📊 Cloud Performance Report ✅ AI verdict: This PR adds PREWHERE support, trivial-count-from-metadata, and real per-column sizes for Memory tables (plus a StorageMerge row-policy fix); ClickBench's benchmark tables use the MergeTree engine, so the server's MergeTree read/aggregation hot path is untouched. The one flagged query, ClickBench Q31 (GROUP BY SearchEngineID, ClientIP), was deterministically kept as not_sure inside master's current variance band, and its history is intrinsically noisy, so the +12.6% here reads as run-to-run variance rather than a real PR effect. No regression is attributable to this change. The new memory_prewhere performance test is where the intended improvement should show up. clickbenchFlagged queries (1 of 43)
Change = percent below ×2; the ratio of medians (×N faster/slower) beyond, where percent understates the scale. q-value = BH-FDR adjusted p; smaller is stronger evidence. MIRAI flags a query when q < fdr_q (default 0.10) — the value the verdict is based on. tpch_adapted_1_official🟢 No significant changes Debug info
|
Virtual columns (e.g. `_table`) are materialized outside the reading source, so the in-source filter cannot read them: `SELECT * FROM t PREWHERE _table = 't'` failed with `NOT_FOUND_COLUMN_IN_BLOCK`. Declare `supportedPrewhereColumns`, so both the analyzer and the plan-level WHERE -> PREWHERE optimization reject such conditions with `ILLEGAL_PREWHERE`, as asserted by `03094_virtual_column_table_name`. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=116248&sha=ebb9ae35257ee1588af487bfadfbb6002ac9c7d6&name_0=PR&name_1=Fast%20test Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ake) Generated by running the tests; the values match manual computation and the outputs observed in the CI report https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=116248&sha=ebb9ae35257ee1588af487bfadfbb6002ac9c7d6&name_0=PR&name_1=Fast%20test Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- `03610_disjunctions_pushdown_optimization` already pins `optimize_move_to_prewhere` and `query_plan_optimize_prewhere` to 1, so the pushed-down disjunctions over `Memory` tables now become PREWHERE; update the expected plan accordingly. - `03777_join_precalculate_keys` and `03707_analyzer_convert_outer_any_to_inner` assert on join plans, so pin `optimize_move_to_prewhere = 0` to keep the asserted plans independent of the (harness-randomized) PREWHERE move. - `03562_short_circuit_for_and_or` asserts on `read_rows` of count subqueries to prove short circuit; pin `optimize_trivial_count_query = 0`, because serving the count of a `Memory` table from metadata would defeat that signal. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=116248&sha=ebb9ae35257ee1588af487bfadfbb6002ac9c7d6&name_0=PR&name_1=Fast%20test Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
The official master build is compiled with Object file sizes9 object files changed (+72.21 KiB total), 0 added.
716 more object files are built by the master warmup baseline only (it builds every object-file target, a pull request build only Compile time of recompiled translation units16 translation units recompiled, 106 s compile time in total, 16 of them have a recent master baseline. |
…ocks `SELECT count()` on a `Memory` table is served from the row counter, while ordinary reads use the set of blocks captured in `getStorageSnapshot`. The counters were separate atomics updated around `data.set`, so a concurrent reader could observe a row count that corresponded to no state the table ever had. Move the row and byte counters into the `MultiVersion` object that holds the blocks, as the pre-existing `TODO` in `getStorageSnapshot` suggested. They are now published atomically together with the blocks they describe, `totalRows` is the exact row count of a committed state, and `SnapshotData::rows` is exact rather than approximate. Also restrict `supportedPrewhereColumns` to the stored columns without a `DEFAULT` expression (the same restriction as `StorageFile`): such a column is absent from the blocks written before `ALTER TABLE ... ADD COLUMN`, and the in-source filter reads it as the default value of its type instead of evaluating the expression. `ALIAS` and `EPHEMERAL` columns are excluded as well, because they are never stored. Add `05052_memory_prewhere_added_column` and `05053_memory_trivial_count_concurrent`.
| MemorySourceFilterPtr ReadFromMemoryStorageStep::makeSourceFilter(const NamesAndTypesList & physical_columns) const | ||
| { | ||
| if (!query_info.row_level_filter && !query_info.prewhere_info) | ||
| return nullptr; | ||
|
|
||
| auto result = std::make_shared<MemorySourceFilter>(); | ||
| ExpressionActionsSettings actions_settings(context); | ||
|
|
||
| /// The row-level security filter runs first, so PREWHERE expressions are never evaluated | ||
| /// on the rows the policy hides. | ||
| if (query_info.row_level_filter) | ||
| { | ||
| const auto & row_level_filter = *query_info.row_level_filter; | ||
| result->steps.push_back({ | ||
| .actions = std::make_shared<ExpressionActions>(row_level_filter.actions.clone(), actions_settings), | ||
| .filter_column_name = row_level_filter.column_name, | ||
| .remove_filter_column = row_level_filter.do_remove_column, | ||
| .replace_filter_to_constant = false, | ||
| }); | ||
| } | ||
|
|
||
| if (query_info.prewhere_info) | ||
| { | ||
| const auto & prewhere_info = *query_info.prewhere_info; | ||
| result->steps.push_back({ | ||
| .actions = std::make_shared<ExpressionActions>(prewhere_info.prewhere_actions.clone(), actions_settings), | ||
| .filter_column_name = prewhere_info.prewhere_column_name, | ||
| .remove_filter_column = prewhere_info.remove_prewhere_column, | ||
| .replace_filter_to_constant = !prewhere_info.remove_prewhere_column && prewhere_info.need_filter, | ||
| }); |
There was a problem hiding this comment.
This new source-local filter path executes row_level_filter / PREWHERE actions inside MemorySource::generateFiltered(), but it never prepares the IN (subquery) sets those DAGs can carry. VirtualColumnUtils / PreparedSets require that to happen in applyFilters(), and ReadFromMergeTree also has a follow-up updatePrewhereInfo() hook for conditions that are moved into PREWHERE after applyFilters() already ran.
As written, valid queries such as SELECT ... FROM memory_table PREWHERE k IN (SELECT ...) can still fail with LOGICAL_ERROR: Not-ready Set is passed, and the same problem applies to pushed row-policy filters using IN. I think this step needs the same set-preparation handling as ReadFromMergeTree, plus a focused regression test for explicit PREWHERE ... IN (SELECT ...) and a row policy with IN.
There was a problem hiding this comment.
🕵 Done in 961435020213: makeSourceFilter now calls VirtualColumnUtils::buildSetsForDAGExcludingGlobalIn on query_info.row_level_filter->actions and query_info.prewhere_info->prewhere_actions before it turns them into ExpressionActions. That is a single point covering every path into the source (explicit PREWHERE, a condition moved there by optimizePrewhere after applyFilters already ran, a pushed row policy, and the delay_read_for_global_sub_queries reader), so it does not depend on query_plan_optimize_primary_key being enabled, unlike splitting the work between applyFilters and updatePrewhereInfo as ReadFromMergeTree does. Sets of GLOBAL IN are excluded for the same reason as in ReadFromMergeTree::updatePrewhereInfo: ReadFromRemote has to attach an external table to them first.
One correction to the finding: I could not reproduce an actual failure. On a binary built without this change, PREWHERE k IN (SELECT ...), NOT IN, the optimizer-moved variant, use_index_for_in_with_subqueries = 0, a self-referencing subquery, Merge over Memory, and a row policy USING k IN (SELECT ...) all return correct results, because the pipeline-level CreatingSetsStep does gate the source through DelayedPortsProcessor in those plans. So this is the same hardening ReadFromMergeTree carries against the short-circuit race, not a currently reachable Not-ready Set is passed.
The regression test is 05057_memory_prewhere_in_subquery, covering explicit PREWHERE ... IN (SELECT ...), NOT IN, the optimizer-moved form, and a row policy with IN combined with both PREWHERE and trivial count().
There was a problem hiding this comment.
This still runs too late to make the IN sets ready on the source-local path.
src/Interpreters/PreparedSets.h:156-159 and src/Storages/VirtualColumnUtils.h:32-39 both require the in-place build to happen from SourceStepWithFilter::applyFilters(): once the plan reaches pipeline construction, CreatingSetsStep may already own the subquery. That is exactly what happens here. QueryPlan::optimize() expands DelayedCreatingSetsStep before pipeline build (src/Processors/QueryPlan/Optimizations/optimizeTree.cpp:875-896), addPlansForSets() asks each FutureSetFromSubquery for a plan (src/Processors/QueryPlan/Optimizations/addPlansForSets.cpp:15-33), and FutureSetFromSubquery::build() does auto plan = std::move(source) (src/Interpreters/PreparedSets.cpp:430).
So by the time ReadFromMemoryStorageStep::makeSourceFilter() reaches buildSetsForDAGExcludingGlobalIn here, buildOrderedSetInplace / buildSetInplace no longer have a source plan to execute. Non-key PREWHERE ... IN (SELECT ...) and pushed row-policy filters therefore still fall back to the pipeline-level CreatingSetsStep, so the downstream short-circuit race this hardening is trying to close remains reachable.
…er test `ReadFromMemoryStorageStep` evaluates the row-level security filter and `PREWHERE` inside `MemorySource`, so a condition such as `PREWHERE k IN (SELECT ...)` carries a `FutureSet` that has to be ready by the time the source runs. The pipeline-level `CreatingSetsStep` normally fills it in, but `DelayedPortsProcessor` can be short-circuited by a downstream processor that closes its inputs early, which is why `ReadFromMergeTree` builds those sets in place for its storage-level `PREWHERE`. Do the same in `makeSourceFilter`, excluding the sets of `GLOBAL IN`, to which `ReadFromRemote` still has to attach an external table. Added `05057_memory_prewhere_in_subquery` covering explicit and optimizer-moved `PREWHERE ... IN (SELECT ...)` and a row policy with `IN`. `05052_memory_prewhere_added_column` failed with the old analyzer: it substitutes an `ALIAS` column expression into `PREWHERE` before the storage sees it, so `PREWHERE a = 4` is not rejected there. Pin `enable_analyzer` for that assertion. Renumbered the two tests that collided with master. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=116248&sha=de59dc98eb76385347efe69e31ca5bf9b02bdf47&name_0=PR&name_1=Stateless%20tests%20%28amd_llvm_coverage%2C%20old%20analyzer%2C%20s3%20storage%2C%20DBReplicated%2C%20parallel%2C%202%2F3%29 #116248
clickhouse-gh
Bot
added
the
comp-simple-engines
…a source table has a row policy `StorageMerge::supportsTrivialCountOptimization` only asked the source tables the same question, and `StorageMerge::totalRows` sums their `totalRows`. The row policy of a source table is applied later, while `createChildrenPlans` builds the child read plan (`RowPolicyData`), and it is not reflected in the source table's `totalRows`, so `SELECT count()` from the `Merge` table returned the count including the rows the policy hides. This is reproducible on `master` with a source table of a storage that advertises the trivial count unconditionally, e.g. `File`: ``` CREATE TABLE file_child (x UInt64) ENGINE = File(TSV); INSERT INTO file_child SELECT number FROM numbers(10); CREATE TABLE merge_over_file (x UInt64) ENGINE = Merge(currentDatabase(), '^file_child$'); CREATE ROW POLICY pol ON file_child USING x < 3 TO ALL; SELECT count() FROM file_child; -- 3 SELECT count() FROM merge_over_file; -- 10, must be 3 ``` For a source table of the `MergeTree` family the gap is masked by `apply_patch_parts`, which is enabled by default and makes `MergeTreeData::supportsTrivialCountOptimization` decline for the snapshot-less check `StorageMerge` performs. Making `Memory` support the trivial count opens the gap for `Memory` source tables as well, so close it in `StorageMerge`: decline when any source table has a row policy that is not always true. The row policy of the `Merge` table itself is already checked by the caller.
…in the source `ISource` derives the read progress from the returned chunk, which for the in-source filter holds only the rows that passed, and nothing at all for a block the filter eliminates completely. That under-reported `read_rows` and `SelectedRows` and weakened `max_rows_to_read` and read quotas for a selective scan of a `Memory` table. Report the progress explicitly in `MemorySource::generateFiltered` for every scanned block, including the blocks where no row passes: the number of rows scanned, and the size of the columns actually materialized from the block. This suppresses the automatic accounting of `ISource` (it only kicks in when the generator reported nothing), so the rows are not counted twice, and it makes the number of rows the same as before the in-source filter existed - and the same as what `ReadFromMergeTree` reports for its `PREWHERE`.
…nalyzer
`InterpreterSelectQuery` read the `MergeTree` parts for the condition
selectivity estimator with an `assert_cast` of `storage_snapshot->data`, which
in a release build is a plain `static_cast`. `MergeTreeData::SnapshotData` and
`StorageMemory::SnapshotData` are the only two types of storage snapshot data,
and they alias: the `size_t rows` of the latter sits at the offset of the
`RangesInDataPartsPtr parts` of the former. Until now this was unreachable,
because `StorageMemory` was the only storage with its own snapshot data and it
did not allow moving conditions to `PREWHERE`; every other storage that does
leaves `storage_snapshot->data` empty.
Making `Memory` support `PREWHERE` opened this path, and a table with one row
made the cast produce `parts = 0x1`, which passed the null check and was
dereferenced:
Address: 0x1. Access: <not available>. Address not mapped to object.
...
src/Interpreters/InterpreterSelectQuery.cpp:908: DB::InterpreterSelectQuery::InterpreterSelectQuery(...)::$_0::operator()(bool) const
It fired on the existing test `04927_date_preimage_result_correctness`, whose
`Memory` table is queried with a `WHERE` and `enable_analyzer = 0`. The new
tests of this pull request all used an explicit `PREWHERE` with the old
analyzer, and the crash needs a `WHERE` and no `PREWHERE`.
Check the type of the snapshot data instead, the same way
`ReadFromMergeTree::createProjectionQueryPlan` does. The parts are only used by
the `MergeTree` condition selectivity estimator, so leaving them empty for
other storages is the correct behaviour, not a fallback.
Report: https://s3.amazonaws.com/clickhouse-test-reports/praktika.html?PR=116248&sha=961435020213aeb0283b85946de22b833445e67b&name_0=PR&name_1=Stateless%20tests%20%28arm_binary%2C%20parallel%29
… the read progress `05136_merge_trivial_count_row_policy`: `SELECT count()` from a `Merge` table whose source table has a row policy - for a `Memory` source table, and for a `File` source table, where the wrong result is reproducible on `master`. `05137_memory_prewhere_old_analyzer`: the WHERE -> PREWHERE move for a `Memory` table with `enable_analyzer = 0`, which used to be a segmentation fault. The new tests of this pull request used an explicit `PREWHERE` with the old analyzer, and the crash needs a `WHERE` and no `PREWHERE`. `05138_memory_prewhere_read_rows`: `read_rows` of a selective `PREWHERE` over a `Memory` table is the number of scanned rows, both when one row passes and when no row passes at all. Also fold the row policy of the target of a matched `Alias` table into the comment of `StorageMerge::supportsTrivialCountOptimization`: it needs no check of its own, because `StorageAlias` declines the trivial count for the snapshot-less check that `StorageMerge` performs.
# Conflicts: # tests/queries/0_stateless/03707_analyzer_convert_outer_any_to_inner.sql
`05138_memory_prewhere_read_rows` looked up the queries in `system.query_log` with `query LIKE '%FROM t_memory_read_rows %'`, and the third query of the test ends with `FROM t_memory_read_rows;`, so the trailing space excluded it and only two of the three expected `read_rows` values were returned. Dropped the trailing space from the pattern. `04330_join_disjunctions_pushdown_using_type_mismatch` asserts on the plan of a join over two `Memory` tables. Now that `Memory` supports `PREWHERE`, the filters that the disjunction push-down places above the reads are moved into `PREWHERE` and print as `Prewhere filter column:` instead of `Filter column:`, and both `optimize_move_to_prewhere` and `query_plan_optimize_prewhere` are randomized by the test harness, so the assertion was not deterministic either way. Pinned both to 0 in the two `EXPLAIN` queries, which keeps the reference of the test as it is on master. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Related: ClickHouse/ClickBench#1590
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Support
PREWHERE(including the automatic move ofWHEREconditions byoptimize_move_to_prewhere) forMemorytables: only the columns of the conditions are read at first, and the remaining columns are read only for the blocks where some rows pass, and only for the passing rows. This is especially beneficial for tables withSETTINGS compress = true, because for a selective condition most columns are never decompressed. Additionally,SELECT count() FROM tableon aMemorytable is now served from metadata, andsystem.columnsshows real per-column sizes forMemorytables.The motivation is benchmarking a compressed in-memory table on ClickBench (ClickHouse/ClickBench#1590), where the last seven queries (
CounterID = 62) lose the primary index ofMergeTreeand previously had to decompress every referenced column of every block.Implementation:
StorageMemory::supportsPrewhereis now true.MemorySourceapplies the pushed-down row-level security filter and PREWHERE inside the reading source: it materializes only the filter-input columns, executes the filter steps, skips a block entirely when no row passes, and reads the remaining columns only for the surviving rows (IColumn::filterwith the combined mask). The block layout is kept in exact correspondence with the output header, whichSourceStepWithFilter::applyPrewhereActionsbuilds by running the same actions on the sample block.StorageMemory::getColumnSizesreports real per-column in-memory sizes (compressed sizes whencompress = true). This is what enables the plan-levelWHERE->PREWHEREoptimization (it declines on storages with no column sizes) and letsMergeTreeWhereOptimizerorder conditions by the actual cost of reading their columns.StorageMemory::supportsTrivialCountOptimizationis now true, guarded against tables that are filled during query execution (materialized CTEs,GLOBALsubquery temporary tables) and against pinned snapshots (atomicCREATE MATERIALIZED VIEW ... POPULATE), wheretotalRowsmust not be observed at planning time.MemorySourcereports the read progress explicitly: the automatic accounting ofISourceuses the returned chunk, which holds only the rows that passed the filter, and nothing at all for a block the filter eliminated completely.read_rows,SelectedRows,max_rows_to_readand the read quotas see the number of scanned rows, the same as before and the same as whatReadFromMergeTreereports for itsPREWHERE.Two bugs of other code that this change makes reachable are fixed here as well:
InterpreterSelectQueryread theMergeTreeparts for the condition selectivity estimator with anassert_castofstorage_snapshot->data, which is a plainstatic_castin a release build.MergeTreeData::SnapshotDataandStorageMemory::SnapshotDataare the only two types of storage snapshot data and they alias: the row count of the latter sits at the offset of the parts pointer of the former. It was unreachable, becauseStorageMemorywas the only storage with its own snapshot data and it did not allow moving conditions toPREWHERE. MakingMemorysupportPREWHEREturned it into a segmentation fault on the WHERE -> PREWHERE move withenable_analyzer = 0.StorageMerge::supportsTrivialCountOptimizationonly asked the source tables the same question, while the row policy of a source table is applied later, whencreateChildrenPlansbuilds the child read plan, and is not reflected in the source table'stotalRows.SELECT count()from theMergetable therefore counted the rows the policy hides. This is reproducible onmasterwith aFilesource table; for a source table of theMergeTreefamily it is masked byapply_patch_parts, which is enabled by default and makesMergeTreeData::supportsTrivialCountOptimizationdecline for the snapshot-less checkStorageMergeperforms.Benchmark (ClickBench queries, 10M-row
hitssubset in aMemorytable withcompress = true, 96-core aarch64, hot runs, new binary with the optimizations toggled off/on viaoptimize_move_to_prewhere/optimize_trivial_count_query):SELECT COUNT(*)SELECT * ... URL LIKE '%google%' ORDER BY ... LIMIT 10WHERE CounterID = 62 AND EventDate ...SELECT *point lookup byWatchIDThe remaining queries are unchanged within noise. The effect grows with table size and filter selectivity: on the full 100M-row dataset the eliminated blocks dominate.
Workflow [PR]
Sync PR [sync-upstream/pr/116248]