[fix](be) Scope Parquet bloom cache to row groups#65489
Closed
Gabriel39 wants to merge 1 commit into
Closed
Conversation
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: Parquet row-group pruning cached Bloom filters only by leaf column id. After the first row group loaded a column Bloom filter, later row groups reused that filter and could be incorrectly pruned when a matching value existed only outside the first row group. Scope cache entries by both row group and leaf column so absence checks always use the current row group's Bloom filter. Add a parquet-mr fixture with two row groups and real Bloom data plus an enabled/disabled differential unit test.
### Release note
Fix incorrect Parquet Bloom filter pruning that could silently omit rows from later row groups.
### Check List (For Author)
- Test: Unit Test
- ParquetStatisticsPruningTest.BloomFilterCacheIsScopedToRowGroupAndColumn
- Behavior changed: Yes, Parquet Bloom pruning now uses each row group's own filter
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review summary for PR #65489.
I did not find a substantiated blocking issue or any inline comment to submit.
Critical checkpoint conclusions:
- Goal and proof: The PR fixes Parquet row-group bloom pruning by changing the v2 metadata-pruning cache from column-only to
(row_group_idx, leaf_column_id), which matches the described failure mode. The added BE unit test covers the enabled/disabled bloom-pruning differential with a two-row-group fixture. - Scope: The code change is small and focused on
RowGroupBloomFilterCache, plus a targeted test and binary fixture. - Concurrency and lifecycle: The cache is local to one metadata-planning call and is not shared across threads. No special static/global lifecycle or new ownership cycle was introduced.
- Configuration: No new config was added. Existing
enable_parquet_filter_by_bloom_filterpropagation remains the gate for the v2 scan path. - Compatibility: No persisted format, protocol, or FE/BE contract changed. The added Parquet file is test data only.
- Parallel paths: The older Parquet reader uses a cache scoped inside a single-row-group
_process_column_stat_filtercall, and the metadata table bloom probe reads per-row-group bloom metadata directly, so I did not find a parallel missing fix. - Test coverage and results: Test coverage is targeted to the regression. I could not run BE UTs in this checkout because
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protocare missing; validation here was static review plus GitHub patch metadata. - Observability and performance: Existing pruning stats remain intact. The wider cache key slightly increases entries only to the natural row-group/column granularity needed for correctness.
- Transaction, persistence, data writes, and FE-to-BE variable passing: Not applicable to this PR.
User focus: No additional user-provided review focus was present.
Subagent conclusions:
optimizer-rewritefound no optimizer/rewrite, semantic-equivalence, join/aggregate, or parallel-path issue.tests-session-configfound no deterministic test, fixture compatibility, session/config propagation, style, or likely CI/build issue.- Convergence round 1 ended with both live subagents replying
NO_NEW_VALUABLE_FINDINGSfor the same empty proposed inline comment set.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
Superseded by #65500, which consolidates and validates the four related fixes. |
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
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.
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary:
Parquet row-group pruning cached Bloom filters only by leaf column ID. Once a column Bloom filter was loaded for the first row group, subsequent row groups reused that filter. An equality or IN predicate could therefore prune a later row group based on an absence result from the first row group's Bloom filter, silently omitting matching rows.
This change scopes both the cached Bloom filter and its loaded state by
(row_group_idx, leaf_column_id), ensuring every absence decision uses the current row group's filter.The test fixture was generated by parquet-mr 1.17.0 with four INT32 rows split into two row groups, real Bloom filters enabled for
id, and statistics/dictionary encoding disabled. The unit test verifies both row groups expose Bloom filters and compares pruning with Bloom disabled and enabled. A value present only in RG1 keeps RG1 selected while RG0 is pruned.Release note
Fix incorrect Parquet Bloom filter pruning that could silently omit rows from later row groups.
Check List (For Author)
ParquetStatisticsPruningTest.BloomFilterCacheIsScopedToRowGroupAndColumnstddef.hlookup and existing NOLINT/readability findings)