Fix stream ordering bug in join of expression-based keys - #23024
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesJoin evaluation fixes
Broadcast stream synchronization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
pentschev
left a comment
There was a problem hiding this comment.
LGTM, thanks Lawrence!
| left = _apply_casts(left, left_casts) | ||
| right = _apply_casts(right, right_casts) |
There was a problem hiding this comment.
This potentially delivers new columns on left's (resp. right) stream, so can't be inside the context manager.
| left_on = DataFrame( | ||
| broadcast( | ||
| *(e.evaluate(left) for e in left_on_exprs), stream=left.stream | ||
| ), | ||
| stream=left.stream, | ||
| ) | ||
| right_on = DataFrame( | ||
| broadcast( | ||
| *(e.evaluate(right) for e in right_on_exprs), stream=right.stream | ||
| ), | ||
| stream=right.stream, | ||
| ) |
There was a problem hiding this comment.
expr.evaluate runs on the input DataFrame's stream, so we now run it on the respective side's stream before getting a joined stream to actually do the join on.
| # global_agg_df and chunk_df may live on different streams. Since we do | ||
| # an evaluation of values in chunk_df via Expr.evaluate, run the | ||
| # broadcast on chunk_dfs stream, making sure the global_agg stream | ||
| # waits. | ||
| with stream_ordered_after( | ||
| lambda: chunk_df.stream, upstreams=[global_agg_df.stream] | ||
| ) as stream: | ||
| result_cols = [ | ||
| _broadcast_gw_sync( | ||
| ne.value, chunk_df, global_agg_df, key_names, stream | ||
| ).rename(ne.name) | ||
| if isinstance(ne.value, GroupedWindow) | ||
| else ne.evaluate(chunk_df, context=ExecutionContext.FRAME) |
There was a problem hiding this comment.
Both _broadcast_gw_sync and ne.evaluate return some expressions on chunk_df.stream, so we can't run those on a stream joined with chunk_df and global_agg_df. Instead, join global_agg_df against chunk's stream and use that.
When join keys are expressions, and not just simple column references, then evaluation of them might launch new kernels. Since Expr.evaluate does not take a stream (it gets the stream from the input DataFrame) we must launch the evaluation of input keys before the stream_ordered_after context manager. Otherwise, although accessing the to-be-join tables is safe on the join stream, accessing the keys is not: they are on the left and right streams respectively with (potentially) new work queued up. To fix this, just compute the key columns on their respective table's stream before obtaining a joined stream for the join. - Closes NVIDIA#22967
The same problem applied, Expr.evaluate always delivers expressions on its input DataFrame's stream.
78052be to
13f576b
Compare
|
/merge |
Description
When join keys are expressions, and not just simple column references, then evaluation of them might launch new kernels. Since Expr.evaluate does not take a stream (it gets the stream from the input DataFrame) we must launch the evaluation of input keys before the stream_ordered_after context manager. Otherwise, although accessing the to-be-join tables is safe on the join stream, accessing the keys is not: they are on the left and right streams respectively with (potentially) new work queued up.
To fix this, just compute the key columns on their respective table's stream before obtaining a joined stream for the join.
test_join_on_expression_conditionsin cudf-polars-polars tests #22967Checklist