feat: add ASOF join logical semantics - #23829
Conversation
|
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
|
cc @2010YOUY01 & @jayzhan211, we can work on this PR now! |
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.
| left: Arc<LogicalPlan>, | ||
| right: Arc<LogicalPlan>, | ||
| on: Vec<(Expr, Expr)>, | ||
| match_condition: AsOfMatch, |
There was a problem hiding this comment.
nit and optional: would it be better to use Expr instead of AsOfMatch?
The reason is that the typed AsOfMatch can only do part of the validation. It guarantees the expr has the form expr1 comparator expr2, but later we still need to validate expr1 and expr2 further. I think keeping them in one place might be better — the "scattered implementation" pattern is bad in most cases.
This might just be personal taste though. I don't see it as a major issue, happy to be convinced otherwise.
There was a problem hiding this comment.
I'd prefer to keep AsOfMatch. It gives the planner and downstream APIs the left/op/right structure they all need; using a generic Expr would make each consumer unpack the binary form again. Schema-dependent checks still belong in AsOfJoin::try_new.
Sure, take your time! I have moved some independent optimizations in to dedicated PRs as follow-ups. |
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.