Antalya 25.8 Backport of #87303 - Fix condition not being moved to PREWHERE in case there is a row policy (version 2) - #2171
Conversation
25.8.15 Backport of ClickHouse#87303 - Fix condition not being moved to PREWHERE in case there is a row policy (version 2)
), and fix two-step PREWHERE in the Parquet v3 reader PR #1345 backported upstream ClickHouse#87303, which lifts row-level security out of PrewhereInfo into SelectQueryInfo::row_level_filter. Upstream then had to fix several places that were left reading row-level security off prewhere_info. None of those fixes are in #1345, so backport them here, and fix a crash that ClickHouse#87303 makes reachable on this branch. Parquet::Reader::applyPrewhere could not run two filtering steps. Every block it assembles holds rows_pass rows - the count surviving all previous steps - but the function got that wrong in two ways: * Columns were materialized lazily per step via formOutputColumn, which for a primitive column takes the decoded subchunk. The decoders only saw the filter as it stood before any step ran, so that subchunk still holds the pre-filter row count, while the per-step filtering only shrinks what is already in row_subgroup.output - pending subchunks are never touched. A column first needed by the second step therefore arrived one filter generation behind and tripped chassert(filter.size() == row_subgroup.filter.rows_pass). Materialize every step's inputs before running any step so they are filtered in lockstep. formOutputColumn moves out of the subchunk, so this only shifts ownership earlier and does not change peak memory. * addDummyColumnWithRowCount was passed rows_total, and it asserts that every column already in the block has exactly that many rows. That held only because of the bug above, which left the second step's column unfiltered; with the columns correctly at rows_pass it fails instead. Pass rows_pass, which is the row count the block actually has at every step. Planner: ClickHouse#87303 also replaced the pre-existing add_filter gate (canMoveConditionsToPrewhere && optimize_move_to_prewhere && supportedPrewhereColumns->contains(...) && !has_table_virtual_column) with a bare supportsPrewhere() for the row policy, dropping the supportedPrewhereColumns() check. StorageFile, IStorageURLBase and StorageObjectStorage all restrict prewhere to physical columns, while a row policy's filter column is usually an expression name, so before ClickHouse#87303 those storages routed the policy to WHERE. Upstream release branches do not notice the loss because input_format_parquet_use_native_reader_v3 defaults to false there, making supportsPrewhere() false for Parquet anyway; this branch enables that reader by default. Restore the check for the row policy only. MergeTree returns nullopt and is unaffected, so the move-to-prewhere fix that motivates the backport is preserved, and its prewhere steps are executed by MergeTreeRangeReader rather than by the code above. The two fixes cover different shapes. The guard diverts expression-valued policies, which is the common case. A policy whose condition is a bare column (USING flag) is named after a physical column, passes the guard, and still reaches the reader: with an explicit PREWHERE that shape aborted on this branch even before ClickHouse#87303, and after ClickHouse#87303 a plain WHERE moved into prewhere aborts too, so the reader fix is needed as well. The applyPrewhere limitation is present on every upstream release branch carrying ClickHouse#87303 and was only fixed on master, by the multistage-prewhere redesign (ClickHouse#93542); the fix here is local to this branch and worth offering upstream separately. updateFormatPrewhereInfo, two upstream commits that must go together: * 8ddee54, "Fix exception in updateFormatPrewhereInfo when only row_level_filter is set": the assertion still required prewhere_info, but every caller now invokes the function when either filter is set, so a row policy without PREWHERE on an object storage / File / URL table tripped it. row_level_filter was also never stored into the new ReadFromFormatInfo and got lost. * 774b56b, "Fix updateFormatPrewhereInfo called more than once when row policy and prewhere are both active": storing row_level_filter (above) makes the duplicate-call guard reject a legitimate second call. When a table has a row policy and the optimizer later pushes WHERE into PREWHERE, the function runs twice - once from read() for the row_level_filter, once from updatePrewhereInfo() for both. Guard only against duplicate prewhere_info, and skip re-applying a row_level_filter that a previous call already applied. * 92b0d17, "Consider row level filter for read in order optimization": the row-level filter expression was no longer appended to the sorting DAG, so its fixed columns were not recognised and read-in-order was skipped; the limit was also no longer reset despite filtering being present. * 25c22b7, 6b35e27, "Fix row policy filter error when using projections" / "Fix for NOT_FOUND_COLUMN_IN_BLOCK when selecting from projections": projection_query_info kept row_level_filter while projectionsCommon already folds it into the projection prewhere, so the filter was applied twice and failed on the projection's block layout. Tests come from the upstream commits verbatim, except: * 04490_row_policy_parquet_v3_two_prewhere_steps is new and specific to this branch: it covers both shapes above on a File(Parquet) table with the v3 reader, and pins the routing guard for expression-valued policies. * 03800_projection_row_policy_filter_column.reference: its EXPLAIN indexes=1 output has "Ranges: 1" indented two spaces deeper on this branch, because ReadFromMergeTree::describeIndexes still prints it with an extra indent level here. Regenerated against this branch; no other byte differs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
This one was quite bumpy. I've had two files with merge conflict. Resolved them with Claude. After that I revealed that some of the tests actually crashed the server. All of that I covered with the second commit. Third commit is aligning the test output. The tests now pass locally, but something else might fail in CI. If it does, I'll fix it promptly. |
Both tests were imported from the commits that introduced them (25c22b7, 92b0d17), but upstream hardened them afterwards, in both cases because of the failures we hit: * 6251342, "Disable parallel replicas for test" (same day 03927 landed): adds SET enable_parallel_replicas = 0. clickhouse-test randomizes parallel_replicas_local_plan, and with no local plan there is no local ReadFromMergeTree, so the ReadType lines the test greps for disappear. * c70a81c, "Fix flaky 03800 RLS+projection test under ParallelReplicas", plus 5c5e975, 2954a15 and 8473072: disables parallel replicas on the EXPLAIN queries for the same reason, pins index_granularity because the EXPLAIN indexes section asserts an exact granule count, adds a baseline query without the row policy so the result demonstrably changes once the policy applies, and adds two assertions that do not depend on plan indentation - a count() > 0 check that the projection was read, and an extract() of the equals(tenant_id, ...) predicate showing the policy is applied as a prewhere filter on the projection. Both files are upstream/master verbatim except for SET explain_query_plan_default, which selects between the legacy and pretty EXPLAIN plan formats and does not exist on this branch - it was added upstream on 2026-05-20, and only the legacy format exists here. 03800's reference is regenerated against this branch: it differs from upstream only in "Ranges: 1" being indented two spaces deeper, because ReadFromMergeTree::describeIndexes still prints it with an extra indent level here. 03927's reference is byte-identical to upstream. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
03800_projection_row_policy_filter_column's two data queries fail with PROJECTION_NOT_USED under the ParallelReplicas variant. Upstream's enable_parallel_replicas = 0 pins cover only its three EXPLAIN queries. projectionsCommon.cpp reports projection support for the initiator only when parallel_replicas_local_plan is set, so with it 0 optimizeUseNormalProjection skips projection reading on remote replicas and force_optimize_projection = 1 throws. That logic is identical upstream, and so is the randomization of the setting in clickhouse-test - but upstream additionally forces it back to 1 (its clickhouse-test has that override, ours does not). The test itself is therefore not at fault, so blacklist it rather than diverging the file from upstream, which the previous commit had just converged. Note this leaves the underlying gap in place: any other test relying on projections under parallel replicas will hit the same randomization. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI triage for #2171Verdict: 6 failing checks, but only 3 distinct failures (each reported twice — once as the S3-report check-run Strongest evidence up front: the sibling PRs #2168 (crash fix in StorageObjectStorage) and #2172 (unblock TTL part) — neither of which touches the code this PR changes — show the identical trio of failures with the same fingerprints ( 1.
|
Review feedback: a SET at the top of the test is preferable to a parallel_replicas_blacklist.txt entry. It is also upstream's own idiom - 03927 uses exactly this form, from 6251342 "Disable parallel replicas for test" - and it keeps the test running in the ParallelReplicas job rather than excluding it, so projections plus row-level security stay covered there. Reverts the blacklist entry added in 21a65c3. The reason the data queries need this, unlike the EXPLAIN queries upstream already pinned, is that projection reading is skipped on remote replicas unless parallel_replicas_local_plan is set (projectionsCommon.cpp), clickhouse-test randomizes that setting, and force_optimize_projection = 1 then throws PROJECTION_NOT_USED. Note this is still a per-test workaround. The underlying difference is that upstream's clickhouse-test forces parallel_replicas_local_plan back to 1 after randomizing it and this branch's copy does not, so any other test combining projections with force_optimize_projection can hit the same failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback: the test only checked row counts, so it would keep passing even if the row policy stopped reaching the format reader - the two-step case it exists to cover would then not be exercised at all, silently. Add an EXPLAIN assertion per policy shape: * bare-column policy (USING flag): the filter DAG's output is named after a physical column, so it passes supportedPrewhereColumns() and lands in the reader - Row level filter column 1, external Filter column 0, Prewhere filter column 1. * expression-valued policy (USING id <= 4): diverted to WHERE by the guard, so only the prewhere reaches the reader - the mirror image of the above. Both use an explicit PREWHERE so they do not depend on optimizePrewhere choosing to move a WHERE. LIKE is case-sensitive, so '%Filter column:%' does not match the reader's 'Row level filter column:' and the three counters stay distinguishable. Also pin enable_analyzer = 1. It defaults to 1, but clickhouse-test detects a server running with it off (is_old_analyzer_used) to pick .oldanalyzer references, so the test can run under the old analyzer. That matters here because the supportedPrewhereColumns() guard lives in PlannerJoinTree and applies only to the new analyzer: InterpreterSelectQuery routes every row policy to row_level_filter whenever the storage supports prewhere, so the expression-policy assertion would differ. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed the fixes for the review. Thanks! While doing that, Claude also noticed the possible difference in old-vs-new analyzer: Why the two analyzers place row policies differently ClickHouse can enforce a row policy in two ways. It can apply it as a regular filter after the data is read, or it can push it down into the reader so rows are discarded as early as possible. The second is faster, but only some table Whether pushdown is possible depends on two separate things: whether the engine supports early filtering at all, and whether the particular condition is one the reader can evaluate. Some engines only accept conditions over plain stored The old and new analyzers are separate implementations of query planning, and they check different amounts of this. The new analyzer checks both questions. The old one checks only the first, and pushes the policy into the reader whenever The result is that the same query can be planned two ways depending on which analyzer runs. The rows returned are identical — the policy is always enforced, just at a different point in the pipeline. What changes is the shape of the query This only shows up for tables read through the Parquet reader, since that is the only format offering early filtering, and only for policies whose condition is computed rather than a bare column. Everywhere else the two analyzers agree. The practical consequence is that any test asserting plan shape rather than query results has to say which analyzer it expects. |
4 participants
Same as #1345, but for Antalya 25.8, additional fixes included
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fixed move-to-prewhere optimization, which did not work in the presence of row policy (ClickHouse#87303 by @KochetovNicolai)
Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: