andygrove opened a new pull request, #6273: URL: https://github.com/apache/datafusion-comet/pull/6273
Backport of #5828 to `branch-1.0`. Cherry-picked from `4abfd95114d61ad454f9ee269be1615f469f24e2`. The production change in `operators.scala` merged without conflicts and matches upstream. Two test files needed adapting; see "What changes are included" below. ## Which issue does this PR close? Closes #5824 on `branch-1.0`. Listed in #6201. ## Rationale for this change The bug ships in 1.0.0, through the same code as on `main` before #5828: - The `equals` and `hashCode` of `CometHashJoinExec`, `CometBroadcastHashJoinExec` and `CometSortMergeJoinExec` leave out the join type. - `CometBroadcastHashJoinExec` does not record whether it is a null-aware anti join, although it converts them. - `CometExplodeExec` does not record `outer`. So subtrees that differ only in those compare equal once canonicalized. Examples are `EXISTS` vs `NOT EXISTS`, `NOT IN` vs `NOT EXISTS`, and `explode` vs `explode_outer` over the same input. Exchange reuse then serves one from the other's shuffle, and the query silently returns the wrong rows. This happens with default configs on every Spark version, with AQE on or off. ## What changes are included in this PR? The fix is the original one; see #5828 for the details. The join type, the null-aware flag and `outer` become part of plan identity, and `CometTestBase` gains the `assertExchangeReuseOver` helper the new tests use. The adaptations: - The `CometAggregateSuite` hunk is dropped. It only moves #5470's reuse assertion onto the new helper, and #5470 is not on `branch-1.0` yet; its backport is #6272. The two backports merge cleanly in either order, and #5470's test keeps its inline assertion. - `CometJoinSuite` keeps `branch-1.0`'s imports and adds only what the new tests use. On `main` the import block had grown with tests that are not on `branch-1.0`. ## How are these changes tested? The new tests, run locally on `branch-1.0` with the default Spark 4.1 profile and JDK 17: - All 16 pass: 8 in `CometJoinSuite` (three join strategies and the null-aware anti join, each with AQE on and off) and 8 in `CometGenerateExecSuite` (`explode` and `posexplode`, over a column and over `slice`, each with AQE on and off). - The bug is present on `branch-1.0`, and the tests catch it. With `operators.scala` reverted and the tests kept, all 16 fail. For the shuffled hash join without AQE, Spark returns `[0,10]`, `[1,11]` and `[2,12]`, and Comet returns `[0,10]` and `[1,11]` twice each: the `LEFT SEMI` branch's rows served for the `LEFT ANTI` branch too. - The full `CometJoinSuite` and `CometGenerateExecSuite` pass, 83 tests. - `CometTPCDSV1_4_PlanStabilitySuite` and `CometTPCDSV2_7_PlanStabilitySuite` pass, 129 tests, so no TPC-DS golden plan changes. - Scalastyle, through `test-compile` on the default profile, Spotless, and scalafix in CHECK mode on Spark 3.5 pass. I ran them once with all six `branch-1.0` backports from this round applied together. ## Are there any user-facing changes? Queries whose subtrees differ only in join type, in null-aware anti join, or in `explode` vs `explode_outer` now return correct results. Equivalent plans still share one shuffle. There are no config or API changes. -- 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]
