Antalya 25.8 Backport of #87303 - Fix condition not being moved to PREWHERE in case there is a row policy (version 2) - #2171
Open
mkmkme wants to merge 7 commits into
Open
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>
Collaborator
Author
|
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>
Collaborator
Author
Collaborator
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.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: