feat: add ASOF join logical semantics - #23829
Conversation
github-actions
Bot
added
sql
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23829 +/- ##
=========================================
Coverage 81.47% 81.48%
=========================================
Files 1122 1123 +1
Lines 403629 406478 +2849
Branches 403629 406478 +2849
=========================================
+ Hits 328866 331212 +2346
- Misses 55510 55880 +370
- Partials 19253 19386 +133 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
6429c60 to
1a925ce
Compare
1a925ce to
eace163
Compare
github-actions
Bot
removed
physical-expr
|
cc @2010YOUY01 & @jayzhan211, we can work on this PR now! |
jayzhan211
left a comment
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @Xuanwo , LGTM
Thank you @jayzhan211 for the review 🥰 |
There was a problem hiding this comment.
Thank you, this is great. Could you wait a few days? I need more time to review.
After a quick look, I'm wondering whether it's possible to remove the two pure optimizations marked below and add them in follow-up PRs.
They seem quite tricky, and we've had many related bugs before, so we should add more test coverage targeting those specific optimizations, and potentially better explain the mechanism and hidden assumptions in the separate PRs.
Sure, take your time! I have moved some independent optimizations in to dedicated PRs as follow-ups. |
2010YOUY01
left a comment
2010YOUY01
left a comment
There was a problem hiding this comment.
LGTM, thank you.
My main suggestion is to simplify the logical plan builder API, but we could potentially defer that to the SQL integration PR, since it will be easier to validate at that point.
| physical_right, | ||
| join_on, | ||
| match_condition, | ||
| None, |
There was a problem hiding this comment.
Here is something I don't fully understand — it would be great if other reviewers have the knowledge to double-check, or can point me to something that would help me understand it more easily. (Just flagging where I don't have full confidence; I don't think this is a blocker for this PR.)
Specifically, I don't follow the whole lifecycle of projection pushdown.
Let's say the downstream only requires a subset of the columns inside AsOfJoinExec. I imagine the logical optimizer should try to keep the projection list inside the LogicalPlan, so that we can directly build the physical plan node with the required projection indices.
Here, when building the initial physical plan, the projection list is None, and it seems to depend on a later physical optimizer pass to finish the work.
Perhaps there is some room to simplify that process.
There was a problem hiding this comment.
Good catch. I’ll handle that in a separate follow-up.
| on: Vec<(Expr, Expr)>, | ||
| match_condition: AsOfMatch, |
There was a problem hiding this comment.
I think this is the user-facing API (see the example code in the join_on comment above), so it should be easy to use.
Currently it asks callers to manually normalize the args; instead, we should validate them internally.
Perhaps:
on: Expr,
match_condition: Expr,For example, if the SQL input is on t1.v1 < t2.v1, which is not a supported ON clause for an asof join:
- Existing design: we have to do partial validation in the SQL->LogicalPlan binding
- Alternative: we can consolidate the validation in one place
But I think we can proceed as is and potentially change it in the SQL integration PR. It would be obvious which design is better once we try to build a plan from SQL. And we're likely to do that before the next release, so API changes are fine.
| impl AsOfJoin { | ||
| /// Creates an ASOF join and validates its logical contract. | ||
| /// | ||
| /// This is the pre-coercion boundary. The physical ASOF constructor repeats |
There was a problem hiding this comment.
I find Join::try_new() (in the current file) don't do any validation, maybe it's allowed to build invalid logical plan, and defer validation to the physical plan construction?
After SQL integration we can try if we can delete the validation here to simplify it, it's always better if we can consolidate all the related validation to a single place.
There was a problem hiding this comment.
I think AsOfJoin::try_new should stay as the logical invariant boundary, while the physical constructor validates the post-coercion execution contract. #23830 now removes the SQL-side duplicate parsing and delegates it to the builder.
4 participants
Which issue does this PR close?
Rationale for this change
This is the logical-planning layer of the ASOF JOIN stack. It defines the
logical contract and planner behavior separately from the SQL frontend and
serialization formats.
#23828 is merged, so this PR's diff against
mainis the isolated logicallayer. It no longer depends on the optional floating-point follow-up #24375.
What changes are included in this PR?
LogicalPlan::AsOfJoin,AsOfJoin, andAsOfMatch.operators, equality-key types, and USING constraints.
LogicalPlanBuilderentry points and schema construction that preservesboth qualified
USINGkeys while exposing one unqualified wildcard key.pruning, row bounds, and physical planning.
AsOfJoinExecfrom feat: add ASOF join physical operator #23828.owning stack layers add explicit support.
pushdown to perf: push left filters through ASOF joins #24801 so each optimization can be reviewed independently.
Are these changes tested?
Yes:
cargo fmt --allcargo clippy --all-targets --all-features -- -D warningscargo test -p datafusion-expr min_rows_of_joins --all-featurescargo test -p datafusion-substrait asof_join_fails_closed_until_substrait_has_an_extension --all-featuresAre there any user-facing changes?
This adds logical-plan and builder APIs for ASOF joins. SQL syntax, DataFrame
APIs, and plan serialization are intentionally left to dependent stack PRs.
Floating equality keys remain rejected by the merged physical operator unless
the independent follow-up #24375 is also included.
As with any new public
LogicalPlanvariant, downstream exhaustive matches mustadd an arm. The variant is appended so existing variants retain their
PartialOrdordering; maintainers should still treat the enum addition as aRust source-compatibility break.
This PR can be reviewed independently now that #23828 has merged. The
optimization follow-ups #24799 and #24801 are not required by the core ASOF
stack.