feat: add ASOF join physical operator - #23828
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23828 +/- ##
==========================================
+ Coverage 80.66% 81.03% +0.36%
==========================================
Files 1095 1107 +12
Lines 372294 381960 +9666
Branches 372294 381960 +9666
==========================================
+ Hits 300324 309526 +9202
- Misses 54055 54143 +88
- Partials 17915 18291 +376 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2010YOUY01
left a comment
There was a problem hiding this comment.
Thank you! This is a really good start. I have done a quick first pass and left some suggestions.
| vec![ChildStats::At(partition), ChildStats::Skip] | ||
| } | ||
|
|
||
| fn statistics_from_inputs( |
There was a problem hiding this comment.
Just an idea to make this PR smaller, could we use the default implementation here? We could implement it later in a follow-up PR.
There was a problem hiding this comment.
I kept this small override because ASOF has two exact facts the default would discard: the output row count equals the left row count, and unmodified left columns retain their statistics. Right-side column statistics remain unknown. I added a comment to make that scope explicit.
There was a problem hiding this comment.
I see, this makes sense.
|
There is no new commits after the previous review, you may have forgotten to push the local changes 🤔 @Xuanwo |
2010YOUY01
left a comment
There was a problem hiding this comment.
I went over the implementation in detail, and I think it's well organized.
Need to do before merging
Before merging, I think we could add a few basic tests that run the executor and assert the results, just to cover some different cases:
- No equality condition in
on. - Different comparison operators in the match condition, such as
<and>=. - Complex expressions in the
onor match conditions, such asMATCH_CONDITION (l_c1 + l_c2) > r_c1orON l_c1 = (r_c1 + r_c2). I think these should be supported.
Optional suggestions
The main implementation/design questions I have are:
- We need to buffer
batch_sizeoutput rows before emitting them, now it's implemented through thePendingRowsstruct, so we need to buffer all source batches to produce the final output. I think this uses more memory and makes the implementation more complex. An alternative is a) keep ain_progress_batch, and emit it after it reaches threshould b) only buffer one (left_batch, right_batch), and materialize its valid indices into thein_progress_batchwhen the cursor moves across it. This might be fast enough and simpler. - Each loop iteration only advances the right index by one. We can probably explore some fast-forwarding optimization here.
But I suggest we first implement this end to end, including planning, SQL support, more tests, and benchmarks, before exploring these optimizations further. It should be easier to validate the ideas afterward.
For now, I only suggest trying to simplify the existing implementation or adding more documentation to make future iterations easier. I left a few suggestions in the comments.
| /// rows in the same group; left EOF flushes the final pending rows. NULL keys | ||
| /// and group changes clear the candidate, while output flushes only clear | ||
| /// pending row references. | ||
| async fn next_batch(&mut self) -> Result<Option<RecordBatch>> { |
There was a problem hiding this comment.
I recommend to add a pseudocode in the comment for the algorithm implemented in this function.
Besides, since this is the major loop, I think we can also point to this function from execute() and top doc comments, like 'the key state machine entry point is the next_batch() function'
| on: JoinOn, | ||
| match_condition: AsOfMatchExpr, | ||
| /// Sorted, unique indices of right columns appended after all left columns. | ||
| right_output_indices: Vec<usize>, |
There was a problem hiding this comment.
I think this left input is also possible to get projected, we could instead use the existing convention for projection:
|
|
||
| fn input_distribution_requirements(&self) -> InputDistributionRequirements { | ||
| InputDistributionRequirements::new(vec![ | ||
| Distribution::UnspecifiedDistribution, |
There was a problem hiding this comment.
Is it the case the optimizer will insert round-robin repartition, if we declare this UnspecifiedDistribution? It should be fine if it's doing so, I was a little bit confused by this name 🤔
| vec![ChildStats::At(partition), ChildStats::Skip] | ||
| } | ||
|
|
||
| fn statistics_from_inputs( |
There was a problem hiding this comment.
I see, this makes sense.
| } | ||
|
|
||
| #[derive(Clone)] | ||
| struct Candidate { |
There was a problem hiding this comment.
We could add some comments to this struct.
Optional suggestion: I think we could put it inside the right InputCursor to simplify the implementation.
Logically, this represents the "previous valid row," and it seems more like an internal implementation detail of InputCursor than a standalone module. I imagine the state machine code would be easier to understand if we structured it this way.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn broadcasts_right_input_to_all_left_partitions() -> Result<()> { |
There was a problem hiding this comment.
I think this test don't have a meaningful test goal right now, and it adds maintenance overhead.
It would be better to test it by asserting the plan shape, after we have SQL integration. And I think we could remove it now.
| Ok(()) | ||
| } | ||
|
|
||
| #[tokio::test] |
There was a problem hiding this comment.
| #[tokio::test] | |
| // Ensure the build-side memory usage equals the sum of all build-side input | |
| // batches, verifying that the build-side buffer is shared. | |
| #[tokio::test] |
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn preserves_dictionary_outputs_across_large_flush() -> Result<()> { |
There was a problem hiding this comment.
I don't know if we should make such guarantee 🤔
| } | ||
|
|
||
| #[test] | ||
| fn properties_and_statistics_follow_left_preserving_contract() -> Result<()> { |
There was a problem hiding this comment.
Probably it's not necessary to assert the distribution and ordering properties here
| .iter() | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| assert_eq!( |
There was a problem hiding this comment.
Here is a reference to write assertions for similar tests easier:
Which issue does this PR close?
Rationale for this change
This is the first layer of the ASOF JOIN stack. It establishes a
broadcast-based physical execution contract independently so later logical-plan,
SQL, DataFrame, and serialization changes can be reviewed as smaller follow-up
PRs.
The initial implementation deliberately favors the simpler broadcast design:
the right input must fit in memory and each left partition scans the shared
right-side batches. A repartitioned implementation can be evaluated separately
without changing the ASOF semantics introduced here.
What changes are included in this PR?
AsOfJoinExecfor left-preserving, Snowflake-style ASOF semantics.left partitions.
preserve the left-side output partitioning.
batches are zero-copy slices, and expose build, match, and output metrics.
batch boundaries, unmatched rows, invalid contracts, shared-buffer memory
accounting, and multi-partition broadcast execution.
Are these changes tested?
Yes:
cargo fmt --allcargo clippy --all-targets --all-features -- -D warningscargo test -p datafusion-physical-plan joins::asof_join --all-featuresAre there any user-facing changes?
This adds a new physical operator API. SQL and DataFrame APIs are intentionally
left to dependent PRs in the ASOF JOIN stack.