Skip to content

[fix](be) Scope Parquet bloom cache to row groups#65489

Closed
Gabriel39 wants to merge 1 commit into
apache:masterfrom
Gabriel39:codex/fix-parquet-row-group-bloom-cache
Closed

[fix](be) Scope Parquet bloom cache to row groups#65489
Gabriel39 wants to merge 1 commit into
apache:masterfrom
Gabriel39:codex/fix-parquet-row-group-bloom-cache

Conversation

@Gabriel39

Copy link
Copy Markdown
Contributor

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)

  • Test:
    • Unit Test: ParquetStatisticsPruningTest.BloomFilterCacheIsScopedToRowGroupAndColumn
    • BE clang-format v16 check
    • BE clang-tidy attempted; blocked by unrelated remote toolchain/pre-existing diagnostics (stddef.h lookup and existing NOLINT/readability findings)
  • Behavior changed: Yes. Parquet Bloom pruning now uses each row group's own filter.
  • Does this need documentation: No

### 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
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@Gabriel39 Gabriel39 marked this pull request as ready for review July 12, 2026 04:26
@Gabriel39 Gabriel39 requested a review from yiguolei as a code owner July 12, 2026 04:26

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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_filter propagation 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_filter call, 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, and thirdparty/installed/bin/protoc are 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-rewrite found no optimizer/rewrite, semantic-equivalence, join/aggregate, or parallel-path issue.
  • tests-session-config found 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_FINDINGS for the same empty proposed inline comment set.

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 83.33% (5/6) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 56.90% (23543/41373)
Line Coverage 40.57% (230199/567441)
Region Coverage 36.47% (181872/498738)
Branch Coverage 37.55% (81209/216281)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 83.33% (5/6) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 74.68% (30147/40368)
Line Coverage 58.72% (331115/563879)
Region Coverage 55.51% (277885/500634)
Branch Coverage 56.75% (122782/216359)

@Gabriel39

Copy link
Copy Markdown
Contributor Author

Superseded by #65500, which consolidates and validates the four related fixes.

@Gabriel39 Gabriel39 closed this Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants