Skip to content

Apply same sparse routing for range partition dynamic filter as hash partition and restructure build_filter - #24417

Open
peterxcli wants to merge 3 commits into
apache:mainfrom
peterxcli:refactor/structured-shared-bounds-build-filter
Open

Apply same sparse routing for range partition dynamic filter as hash partition and restructure build_filter#24417
peterxcli wants to merge 3 commits into
apache:mainfrom
peterxcli:refactor/structured-shared-bounds-build-filter

Conversation

@peterxcli

Copy link
Copy Markdown
Member

Which issue does this PR close?

per #23854 (comment)

as a follow on it might be nice to break this function into smaller functions (mostly so they can be documented more clearly) -- the techniques here are quite clever

Rationale for this change

  1. Code in SharedBuildAccumulator.build_filter is too complicated and too clever to understand, need to extract it into smaller pieces of logic block.
  2. While trying to refactor the code, I realized my previous pr Enable dynamic filters for range-partitioned joins #23854 didn't follow the sparse routing branches build that hash partitioning is using, because I think I didnt notice that's a thing atm. so I decided to deliever the fix for range partition along with the refactor, because then both range and hash partition dynamic filter expression would have almost similar building structure.

What changes are included in this PR?

  • Apply same sparse routing for range partition dynamic filter that hash partition, so we can share more code among them, now there difference is literally only at the top physical expression to decide which partition should the key go.
  • Extract build_filter in shared_bounds to build_collect_left_filter and build_partitioned_filter, and add comment explanation throughout the code. The most complicated part are filter building fast-path(all_partitions_canceled, all_partitions_empty and one_non_empty_partition) and sparse branches filter building. btw, I skipped documenting the state management for PartitionStatus because I think its code is already self-explanatory.

Are these changes tested?

  • test mod share_bounds
  • filter_pushdown test
  • range_partition.slt

Are there any user-facing changes?

no

@github-actions github-actions Bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Aug 16, 2026
@peterxcli

Copy link
Copy Markdown
Member Author

cc @alamb as you suggest this refactor, @adriangb as you're the author of #18393, #18451 and #21666
cc @gene-bordegaray @jayshrivastava

Please take a look if you have time. TIA

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.17857% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.23%. Comparing base (bbf739a) to head (e17349a).

Files with missing lines Patch % Lines
...physical-plan/src/joins/hash_join/shared_bounds.rs 90.17% 8 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24417      +/-   ##
==========================================
- Coverage   81.23%   81.23%   -0.01%     
==========================================
  Files        1111     1111              
  Lines      390208   390194      -14     
  Branches   390208   390194      -14     
==========================================
- Hits       316990   316968      -22     
- Misses      54591    54598       +7     
- Partials    18627    18628       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants