Conversation
NdFilterPushdown refuses a FilterExec that holds a projection. DataFusion adds a projection when the select list is narrower than the predicate. count(*) gets an empty projection. So the most common spatial query filters after the broadcast. Split the filter. The conjuncts go into NdFilterExec. The projection becomes an NdProjectionExec above it, below the broadcast. A residual conjunct reads the columns that the projection drops, so the projection stays with the residual filter above the broadcast. Keep a filter that holds a fetch. The nd filter has no cap. The old rule dropped the whole node, and the cap with it. Add four unit tests in nd/mod.rs. Each test compares the rewritten plan against the rows of the original plan. Assisted-by: Claude:claude-opus-5
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #401 +/- ##
==========================================
+ Coverage 82.42% 82.46% +0.04%
==========================================
Files 363 363
Lines 59366 59520 +154
==========================================
+ Hits 48931 49086 +155
+ Misses 10435 10434 -1
🚀 New features to boost your workflow:
|
41 tasks
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 changes does this PR make?
NdFilterPushdownnow sinks aFilterExecthat holds a projection. The conjuncts gointo
NdFilterExec. The projection becomes anNdProjectionExecbelow the broadcast.Why do you make these changes?
Closes #397. DataFusion adds a projection when the select list is narrower than the
predicate.
count(*)gets an empty projection. The rule refused such a filter, so theplan broadcast every column onto the full grid first.
How do you make these changes?
A residual conjunct reads the columns that the projection drops. So the projection stays
with the residual filter above the broadcast. The rule also keeps a filter that holds a
fetch, because the nd filter has no cap.Release run on
gridded-example.nc, 2.32 M cells, 714.1 K rows survive:SELECT lat … ST_Within(…)count(*) … ST_Within(…)Does this PR change a public interface or a configuration?
No.
How do you test these changes?
Four unit tests in
nd/mod.rs: a narrow projection, an empty projection, a residualconjunct, and the
fetchguard. Each test compares the rewritten plan against the rowsof the original plan.
I also checked the queries against a runtime with the nd pipeline off. The rows, the
count(*)and thesum()agree.Checklist
cargo fmt --all --checkpasses.cargo clippy --workspace --lib --bins --testsreports no new warnings.cargo test --workspace --lib --bins --testspasses.Notes on the two open boxes:
cargo fmt --all --checkdoes not pass onmaintoday. The new code follows thestyle of the file.
the feature is unreleased.
beacon-core/tests/nd_pipeline_filters.rs. Its testa_narrowing_select_list_keeps_the_filter_above_the_broadcastrecords the oldbehaviour, so it needs an update there.
Was this patch authored or co-authored using generative AI tooling?
Assisted-by: Claude:claude-opus-5