andygrove commented on PR #5470: URL: https://github.com/apache/datafusion-comet/pull/5470#issuecomment-5610598437
Apologies for not reviewing before this PR was merged, but it was only marked as ready for review yesterday. I went through it after the fact and the fix itself looks right to me. I checked out the merge commit and reverted `operators.scala` back to its parent while keeping the new tests, and exactly the five new tests fail, all of them on wrong answers rather than on plan assertions, so the regressions really are targeting the bug. I also tried removing just the `allAttributes` override while keeping the equals change. The same five fail, but this time on `finalAggregate(plus).sameResult(finalAggregate(same))` rather than on results, which confirms both halves are pulling their weight. The equals change buys correctness and the `allAttributes` override buys back the legitimate reuse. That override is also a character for character match of `HashAggregateExec.allAttributes` in Spark 4.1, so no concerns there either. What did come out of the review is that the same defect is still live in four other operators, and I have filed #5824 for it. `CometHashJoinExec`, `CometBroadcastHashJoinExec` and `CometSortMergeJoinExec` all carry `joinType` as a constructor field and all leave it out of `equals` and `hashCode`. `CometBroadcastNestedLoopJoinExec` does include it, which makes me think the other three were simply missed. `LeftSemi` and `LeftAnti` have identical output, keys, condition and build side, so without `joinType` they canonicalize to the same plan. Usually the semi join's inferred `isnotnull(key)` makes the left subtrees differ and hides it, but writing the null check explicitly in both branches is enough to remove that protection. At that point an `EXISTS` / `NOT EXISTS` union returns the `EXISTS` rows twice under Comet and the correct rows under Spark, and it reproduces with all three join strategies. `CometExplodeExec` has the same shape of problem. It writes `op.outer` into the protobuf but never stores it on the case class, so `equals` cannot see it, and `output` does not disambiguate because plain `explode` over an array with `containsNull = true` is already nullable. `InferFiltersFromGenerate` hides this one, but only when the generator input is a bare attribute, so `explode(s.arr)` against `explode_outer(s.arr)` collides on stock configuration. Since you already have the context loaded, would you be up for picking up #5824 as a follow-up? The fixes look like about a line each. The other thing I would like to get out of it is a guard so we stop rediscovering this. A test that reflects over every `CometNativeExec` subclass and asserts that each constructor parameter is either referenced in `equals` or named on an explicit exclusion list would have caught all three of these, including this one, before they shipped. Does that seem worth doing to you? -- 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]
