Skip to content

Fix exception in updateFormatPrewhereInfo when only row-level filter is set#100361

Merged
alexey-milovidov merged 8 commits intomasterfrom
fix-prewhere-info-assertion
Mar 29, 2026
Merged

Fix exception in updateFormatPrewhereInfo when only row-level filter is set#100361
alexey-milovidov merged 8 commits intomasterfrom
fix-prewhere-info-assertion

Conversation

@alexey-milovidov
Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov commented Mar 22, 2026

updateFormatPrewhereInfo asserted that prewhere_info was non-null, but callers in StorageObjectStorage and StorageURL invoke it when either prewhere_info or row_level_filter is set. When a row policy exists on a URL/S3 table with Parquet format (which supports PREWHERE) and the query has no PREWHERE clause, only row_level_filter is populated — triggering the assertion.

Additionally, row_level_filter was never stored into the new ReadFromFormatInfo, so it would be silently lost.

Fix: relax the assertion to accept either being set, and store row_level_filter in the output.

Found in: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=99537&sha=e2a32dfbb945f62e07bd3f66c206a71b584b96b1&name_0=PR&name_1=Stress%20test%20%28amd_debug%29

Changelog category (leave one):

  • CI Fix or Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fix exception "Logical error: prewhere_info" when querying URL/S3 Parquet tables with a row-level security policy and no PREWHERE clause.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

alexey-milovidov and others added 2 commits March 22, 2026 13:01
…er` is set

When `row_level_filter` parameter was added to `updateFormatPrewhereInfo`,
the assertion was not updated — it still required `prewhere_info` to be
non-null. This caused a logical error when querying object storage (S3,
URL, etc.) tables with a row-level security policy but no PREWHERE.

Additionally, `row_level_filter` was never saved into the new
`ReadFromFormatInfo`, so it would be lost.

https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=99537&sha=e2a32dfbb945f62e07bd3f66c206a71b584b96b1&name_0=PR&name_1=Stress%20test%20%28amd_debug%29

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
This test verifies that row policies work correctly on URL tables with
Parquet format (which supports PREWHERE). Previously, this would trigger
"Logical error: 'prewhere_info'" because only `row_level_filter` was set.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 22, 2026

Workflow [PR], commit [224b120]

Summary:


AI Review

Summary

This PR fixes updateFormatPrewhereInfo for file-like/object storages when only row_level_filter is present, and adds a regression test covering URL+Parquet row policy reads (including count()). I did not find correctness, safety, concurrency, or performance regressions in the touched code. The change is focused and consistent with existing ReadFrom* call paths.

ClickHouse Rules
Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate
No magic constants
Backward compatibility
SettingsChangesHistory.cpp
PR metadata quality
Safe rollout
Compilation time
Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label Mar 22, 2026
alexey-milovidov and others added 2 commits March 24, 2026 14:20
…and prewhere are both active

When a table has a row-level security policy (`row_level_filter`) and the optimizer
later pushes a WHERE clause into PREWHERE, `updateFormatPrewhereInfo` was called twice:
first in `read` (for the row_level_filter) and then in `updatePrewhereInfo` (for both).

The guard `if (info.prewhere_info || info.row_level_filter)` rejected the second call
because `row_level_filter` was already stored. The fix:
- Only guard against duplicate `prewhere_info` (the truly non-idempotent part).
- When `row_level_filter` was already applied in a previous call, skip re-applying it
  and only apply the new `prewhere_info` on top.

Also add a `count()` query to the regression test to better exercise the
`need_only_count` code path under row-level filtering.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@clickhouse-gh clickhouse-gh bot added pr-ci and removed pr-bugfix Pull request with bugfix, not backported by default labels Mar 27, 2026
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 29, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.20% 84.20% +0.00%
Functions 24.60% 24.60% +0.00%
Branches 76.70% 76.70% +0.00%

Changed lines: 100.00% (8/8) · Uncovered code

Full report · Diff report

Copy link
Copy Markdown
Member Author

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasonable.

@alexey-milovidov alexey-milovidov self-assigned this Mar 29, 2026
@alexey-milovidov alexey-milovidov merged commit 212e271 into master Mar 29, 2026
152 of 153 checks passed
@alexey-milovidov alexey-milovidov deleted the fix-prewhere-info-assertion branch March 29, 2026 16:01
@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 29, 2026
mkmkme pushed a commit to Altinity/ClickHouse that referenced this pull request Mar 30, 2026
…o-assertion

Fix exception in `updateFormatPrewhereInfo` when only row-level filter is set
Desel72 pushed a commit to Desel72/ClickHouse that referenced this pull request Mar 30, 2026
…o-assertion

Fix exception in `updateFormatPrewhereInfo` when only row-level filter is set
mkmkme pushed a commit to Altinity/ClickHouse that referenced this pull request Mar 31, 2026
…o-assertion

Fix exception in `updateFormatPrewhereInfo` when only row-level filter is set
zvonand added a commit to Altinity/ClickHouse that referenced this pull request Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-ci pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants