Support PREWHERE and trivial count for Memory tables - #116248
alexey-milovidov wants to merge 25 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 [cb57f81] Summary: ❌
AI ReviewSummaryThis PR adds Findings❌ Blockers
Final Verdict
LLVM Coverage ReportMeasured on commit cb57f81.
Changed lines: Changed C/C++ lines covered: 313/334 (93.71%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-performance
|
📊 Cloud Performance Report
Release-transition baseline: compared against the most recent available 26.9 masters at or before the trusted base-commit anchor because no 26.10 master baseline exists there. This PR adds in-source PREWHERE and trivial-count support for Memory tables; the query-execution changes only take effect when a WHERE condition can be moved to PREWHERE or when a bare count() runs. All six flagged ClickBench queries are GROUP BY aggregations, most without a selective filter, so they largely do not exercise the changed path, and several deltas (Q16, Q18, Q33, Q34) sit inside or right on the edge of master's current variance band with noisy history. The one result worth attention is Q32 (GROUP BY URL), a clear and consistent +29.3% that the deterministic gates locked via the large-delta override — it should be re-checked, though the changed code is read/filter-side and not obviously on this query's path. Q16 is downgraded as unrelated noise; the rest are kept as reported. clickbench🔴 1 regressed · Flagged queries (6 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 commit See the job log for details. |
…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`.
…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>
# Conflicts: # tests/queries/0_stateless/03707_analyzer_convert_outer_any_to_inner.sql
…lan optimization `ReadFromMemoryStorageStep::makeSourceFilter` built the sets of the row-level filter and `PREWHERE` from `initializePipeline`. That is too late: at the end of `QueryPlan::optimize`, `DelayedCreatingSetsStep` takes the subquery plan out of every `FutureSetFromSubquery`, so `buildSetInplace` had no source to execute and the sets were still left to the pipeline-level `CreatingSetsStep`, which is exactly the short-circuit race the in-place build is meant to close. Override `applyFilters` and `updatePrewhereInfo` instead, the way `ReadFromMergeTree` does: `applyFilters` builds the sets of the row-level filter and of an explicit `PREWHERE`, `updatePrewhereInfo` builds the set of a condition that `optimizePrewhere` moves into `PREWHERE` afterwards. Sets of `GLOBAL IN` stay excluded. The test asserts on `EXPLAIN PIPELINE`: a set built in place needs neither the `CreatingSet` branch nor the `DelayedPorts` gate, and a control query whose filter stays above the source keeps both. It also runs the shape that motivated the hardening, a downstream `JOIN` with an empty right side. Verified that the test fails on the binary built before this change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The analyzer is mandatory since 26.9 and `enable_analyzer = 0` is rejected with `SETTING_CONSTRAINT_VIOLATION`, so the old-analyzer section of `05043_memory_prewhere`, the whole `05137_memory_prewhere_old_analyzer` test, and the `enable_analyzer = 1` pin in `05055_memory_prewhere_added_column` are gone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
🕵 @groeneai, investigate the failure: https://s3.amazonaws.com/clickhouse-test-reports/praktika.html?PR=116248&sha=3e7700201d2fd04f835203368b83308f1ac2e116&name_0=PR&name_1=Upgrade%20check%20(amd_release) and provide a fix in a separate PR. If the fix is already in progress, link it here. The only red is |
|
Not caused by your change, and the fix is already open: #118499 (2026-09-07, not yet reviewed). The arm is the empty #118499 skips a zero-row part in |
…hdown_use_nulls` The test counts the plan lines matching `Filter column: CAST(%`. `Memory` tables support `PREWHERE` in this branch, so `optimizePrewhere` moves the pushed-down cross-type predicates into the reading step, and `SourceStepWithFilter::describeActions` prints them as `Prewhere filter column: <expression>` - with two spaces, because the pretty expression that `formatFilterPretty` builds already starts with one. The single-space pattern of the test stopped matching those lines, and the counts dropped from 2 and 1 to 1 and 0. Pin `optimize_move_to_prewhere` and `query_plan_optimize_prewhere` off in the three `EXPLAIN` queries, the same way the other plan-asserting tests are pinned in this branch. The queries that assert on results are left alone, so they keep exercising the `PREWHERE` path of a `Memory` table under `join_use_nulls`. https://s3.amazonaws.com/clickhouse-test-reports/praktika.html?PR=116248&sha=fdf6f22ddcb4360e5e590a4521f1b00de73a85e3&name_0=PR&name_1=Fast%20test #116248 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…_privilege` `where_const_view` selects from a `Memory` table with a `WHERE`, and the test asserts on the legacy `EXPLAIN actions = 1` plan, where the condition appears as a `Filter` step directly above `ReadFromMemoryStorage`. With `PREWHERE` support for `Memory` tables the condition moves into the reading step and the whole block changes shape. The test is `no-fasttest` - the encryption functions it needs are not in that build - so the `Fast test` did not catch it. Pin `optimize_move_to_prewhere` off for that one query. The other queries of the test have no `WHERE` at all, so their plans do not move. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| rows += buffer.rows(); | ||
| bytes += buffer.bytes(); | ||
| new_data->rows += buffer.rows(); | ||
| new_data->bytes += buffer.bytes(); |
There was a problem hiding this comment.
BlocksWithCounts::bytes is now part of the committed table state and it is also what the max_bytes_to_keep / min_bytes_to_keep eviction checks compare against. Recomputing it with Block::bytes() here breaks that invariant: inserts and evictions use Block::allocatedBytes(), and those diverge for variable-width columns (ColumnString::byteSize() counts only the used payload, while allocatedBytes() counts the reserved backing storage). After ALTER ... UPDATE/DELETE, totalBytes() can therefore under-report the real in-memory footprint, and the next insert can stop evicting too early because it adds inserted_bytes to an underestimated base. The restore path has the same mismatch at src/Storages/StorageMemory.cpp:696. I think this counter has to use the same metric on every write path, i.e. allocatedBytes() here and in restoreDataImpl().
| /// Sets of `GLOBAL IN` are excluded: `ReadFromRemote` has to attach an external table to them | ||
| /// before they are built. | ||
| if (query_info.row_level_filter) | ||
| VirtualColumnUtils::buildSetsForDAGExcludingGlobalIn(query_info.row_level_filter->actions, context); |
There was a problem hiding this comment.
This still leaves explicit PREWHERE ... GLOBAL IN (subquery) and pushed row policies using GLOBAL IN on the pipeline-built path, even though the predicate is executed inside MemorySource::generateFiltered(). The race described just above is therefore still reachable for those forms: if a downstream processor short-circuits DelayedPortsProcessor, the source will still see a not-ready set. ReadFromMergeTree::applyFilters() builds explicit PREWHERE / row-level sets with buildSetsForDAG() and relies on cannotBeMoved() plus updatePrewhereInfo() to keep only optimizer-moved GLOBAL IN off the synchronous path.
Can we mirror that split here: build all explicit query_info.row_level_filter / query_info.prewhere_info sets in applyFilters(), and keep buildSetsForDAGExcludingGlobalIn() only in updatePrewhereInfo() for the optimizer-moved case? A focused regression on PREWHERE ... GLOBAL IN and a row policy with GLOBAL IN would pin the remaining gap.
2 participants
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]