wangyum commented on PR #59027:
URL: https://github.com/apache/spark/pull/59027#issuecomment-6393891668
Thanks @cloud-fan for the thorough review! All comments from this round have
been addressed in the latest commit:
### 1. Decision Challenges & Trade-offs
For the opt-in internal flag (`spark.sql.join.broadcastRangeJoin.enabled`),
we kept the selection logic straightforward and documented the three trade-offs
in `SQLConf` and the PR description:
- **Interleaved overlap slowdown**: documented per-key-family boundaries
backed by committed benchmark results.
- **Broadcast size gate**: documented that `SizeEstimator` charges retained
index memory, which may cause large hinted builds to fail the broadcast
threshold where BNLJ passed.
- **Tiny probes against large hinted builds**: documented the one-time
driver-side index build overhead.
### 2. Benchmark Cases & Results
- Added 5%, 10%, and 25% staggered overlap cases for both long and double
keys to `RangeJoinBenchmark.scala`.
- Regenerated and updated all three benchmark result files
(`RangeJoinBenchmark-results.txt`, `RangeJoinBenchmark-jdk21-results.txt`,
`RangeJoinBenchmark-jdk25-results.txt`).
- The benchmark results match the documented boundaries:
- **Long keys**: stay near parity up to ~10% overlap (1.1X–1.2X at 5%,
~0.9X at 10%), falling to 0.7X–0.8X at 50%–90%.
- **Boxed (double) keys**: reach parity near ~5% (0.6X–0.9X) and fall to
0.5X–0.6X at 10%–90%.
### 3. Review Findings Addressed
- **Strict BuildLeft exclusive scan count signal**: Added
`checkCandidateCounts` testing `BuildLeft` alongside `BuildRight` with
candidate-count assertions under codegen on and off (`RangeJoinSuite.scala`).
- **Output-row metric coverage**: Added `BroadcastRangeJoin metrics count
only accepted, non-duplicated output rows` covering Inner (with cross-relation
filter `p.p + b.lo <> 25`), Left/Right Outer (with an unmatched probe row),
Left Semi, and Left Anti across codegen on/off (`SQLMetricsSuite.scala`).
- **Config doc slow regime**: Updated `SQLConf.scala` with per-key-family
boundaries and trade-offs.
- **Index builders projection allocations**: Replaced `new
InterpretedProjection(buildKeys)` with direct expression evaluation
(`keyExpr.eval(r)`), eliminating per-build-row `Array[Any]` and
`GenericInternalRow` allocations (`RangeRelation.scala`).
- **Interpreted inner join iterator allocation**: Replaced `flatMap` +
`Iterator.single` with `candidates(row).map(join(row, _)).filter(condition)`
(`BroadcastRangeJoinExec.scala`).
- **Interpreted outer loop stream-reuse ordering**: Added regression test
`outer loop's stream-reuse ordering holds when the join falls out of codegen`
using `range(3)` and `spark.sql.codegen.maxFields=1`
(`RangeJoinSQLSuite.scala`).
- **Boxed inclusive point scans**: Added `<=` and `>=` queries for DOUBLE
and STRING in `probeCoverageQueries`, and added boundary points equal to build
keys in `setupPartialRangeViews` (`RangeJoinSQLSuite.scala`).
- **Comments and fixture names**:
- Corrected midpoint vs $n/2$ hit count comment (`RangeIndexSuite.scala`).
- Corrected tie-break rationale comment
(`ExtractRangeJoinKeysSuite.scala`).
- Renamed `dup_dates` / `probe_dates` to `dup_keys` / `probe_keys` and
updated comments to refer to keys (`RangeJoinSQLSuite.scala`,
`RangeJoinSuite.scala`).
### 4. PR Description
- Updated the walkthrough complexity note ($K \log_2(N/K)$) and buffer-reuse
defense rationale.
- Added the 5%, 10%, 25% staggered benchmark rows to the high-overlap
summary table.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
