You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dongjoon-hyun (via GitHub)" <gi...@apache.org> on 2026/09/22 16:09:26 UTC
[PR] [SPARK-59713] Add a large-group case to AsOfJoinBenchmark [spark]
dongjoon-hyun opened a new pull request, #58969:
URL: https://github.com/apache/spark/pull/58969
### What changes were proposed in this pull request?
This PR adds a third shape to `AsOfJoinBenchmark`: 10K x 10K rows over **10** equi-key groups,
between the existing `groups=100` and no-equi-key cases. `asOfJoinBenchmark` already takes
`numGroups`, so this is purely a new invocation.
### Why are the changes needed?
`SortMergeAsOfJoinScanner` buffers the right-side rows of an equi-key group and rescans that
buffer once per left row, so the scan cost per left row grows with the group size. Neither
existing case covers that: `groups=100` buffers only ~100 right rows per group, where the scan is
swamped by the shuffle and the sort, and the no-equi-key case takes a different planning path
(`AllTuples` distribution, `bufferAllRight` instead of `bufferRightGroup`).
`groups=10` buffers ~1000 right rows per group on the ordinary equi-key path, making changes to
the scan measurable.
### Does this PR introduce _any_ user-facing change?
No. This is a test-only change to a benchmark class.
### How was this patch tested?
Benchmark-only change.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 5
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59713][SQL][TESTS] Add a large-group case to `AsOfJoinBenchmark` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #58969:
URL: https://github.com/apache/spark/pull/58969#issuecomment-5780529455
For the record, CI is running here.
- https://github.com/dongjoon-hyun/spark/actions/runs/35752108314
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59713][SQL][TESTS] Add a large-group case to `AsOfJoinBenchmark` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #58969:
URL: https://github.com/apache/spark/pull/58969#discussion_r4074280533
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/AsOfJoinBenchmark.scala:
##########
@@ -125,6 +125,9 @@ object AsOfJoinBenchmark extends SqlBasedBenchmark {
runBenchmark("AS-OF Join Benchmark") {
// 10K left x 10K right, 100 groups - both paths feasible
asOfJoinBenchmark(leftRows = 10000, rightRows = 10000, numGroups = 100)
+ // Few large groups: each left row scans ~1K buffered right rows, which is where the
+ // per-left-row cost of the right-side group scan dominates.
Review Comment:
Thanks for catching this, @peter-toth - you're right, and I applied your suggestion in ad9e72d.
I confirmed the early return in `findBestBackwardForward`: once `bestMatch` is set, the first row
failing the as-of condition returns immediately, so a left row walks a prefix of the buffer rather
than all of it. Simulating the fixture independently, I get the same magnitudes you did - avg 35.3
comparisons per left row at `groups=100` vs 335.3 at `groups=10`, a 9.5x increase - so the new
case is still the scan-sensitive shape it is meant to be, just with the comment now describing
the right quantity.
(Minor: my count of full-buffer scans came out as 99 and 9 rather than 98 and 8 - the left row
whose `ts` equals its group id has no match for every group except `group_id = 0`. Doesn't affect
the point.)
I left the PR description as is, since "buffers ~1000 right rows per group" is about the buffer
size and is accurate as written.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59713][SQL][TESTS] Add a large-group case to `AsOfJoinBenchmark` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #58969: [SPARK-59713][SQL][TESTS] Add a large-group case to `AsOfJoinBenchmark`
URL: https://github.com/apache/spark/pull/58969
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59713][SQL][TESTS] Add a large-group case to `AsOfJoinBenchmark` [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #58969:
URL: https://github.com/apache/spark/pull/58969#discussion_r4074256256
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/AsOfJoinBenchmark.scala:
##########
@@ -125,6 +125,9 @@ object AsOfJoinBenchmark extends SqlBasedBenchmark {
runBenchmark("AS-OF Join Benchmark") {
// 10K left x 10K right, 100 groups - both paths feasible
asOfJoinBenchmark(leftRows = 10000, rightRows = 10000, numGroups = 100)
+ // Few large groups: each left row scans ~1K buffered right rows, which is where the
+ // per-left-row cost of the right-side group scan dominates.
Review Comment:
**Finding 1.** `findBestBackwardForward` (`SortMergeAsOfJoinExec.scala:385`) stops at the first right row that fails the as-of condition once it already has a match, so a left row scans a prefix of the buffer rather than all of it. The `~1K` figure is the buffer size — which is what the PR description says, "buffers ~1000 right rows per group" — not the scan length.
Simulating the fixture exactly (left `ts = id`, right `ts = id * 3 / 2`, group `id % G`, 10000 rows per side, applying the loop's own termination rule):
```
groups=100 group size=100 total comparisons=353,070 avg per left row=35.3 max=100 full-buffer scans=98
groups=10 group size=1000 total comparisons=3,351,990 avg per left row=335.2 max=1000 full-buffer scans=8
```
So 9.5x the scan work, ~335 rows per left row on average, and the full 1000 only for the 8 left rows that have no match in their group at all — those are the ones where `bestMatch` stays null, so the `else if (bestMatch != null)` early return never fires and the loop runs the buffer out. If you ever want the every-row-scans-the-whole-buffer shape, that is the one to build: a group whose left keys all sort below every right key.
```suggestion
// Few large groups: ~1K right rows buffered per group, and each left row rescans a
// prefix of that buffer, which is where the per-left-row group-scan cost dominates.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59713][SQL][TESTS] Add a large-group case to `AsOfJoinBenchmark` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #58969:
URL: https://github.com/apache/spark/pull/58969#issuecomment-5780627598
Thank you, @peter-toth !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59713][SQL][TESTS] Add a large-group case to `AsOfJoinBenchmark` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #58969:
URL: https://github.com/apache/spark/pull/58969#issuecomment-5780353835
Could you review this benchmark improvement PR, @peter-toth ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59713][SQL][TESTS] Add a large-group case to `AsOfJoinBenchmark` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #58969:
URL: https://github.com/apache/spark/pull/58969#issuecomment-5781261442
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/b6e86c5d7eba69c3ec0f802a95264f530d699d0a
*Posted by `merge_spark_pr.py`*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org