andygrove commented on code in PR #5828:
URL: https://github.com/apache/datafusion-comet/pull/5828#discussion_r3980234043
##########
spark/src/main/scala/org/apache/spark/sql/comet/operators.scala:
##########
@@ -2381,6 +2385,7 @@ object CometBroadcastHashJoinExec extends
CometOperatorSerde[HashJoin] with Come
op.joinType,
op.condition,
op.buildSide,
+ nativeOp.getHashJoin.getNullAwareAntiJoin,
Review Comment:
`createExec` reads the flag back out of the protobuf here, but `doConvert`
has already derived it from the plan a few lines up, and every other argument
in this call comes from `op`. Could `createExec` match on `op` the same way?
```scala
op match {
case bhj: BroadcastHashJoinExec => bhj.isNullAwareAntiJoin
case _ => false
}
```
That keeps one source of truth. The protobuf read also fails quietly rather
than loudly: `getHashJoin` on an unset oneof returns the default instance, so
if a future change stops setting `hash_join` on this path the field silently
becomes `false` and the collision comes back. Your new test would catch that,
so this is about directness more than a live bug.
##########
spark/src/main/scala/org/apache/spark/sql/comet/operators.scala:
##########
@@ -1530,14 +1532,16 @@ case class CometExplodeExec(
this.output == other.output &&
this.generator == other.generator &&
this.generatorOutput == other.generatorOutput &&
+ this.outer == other.outer &&
this.child == other.child &&
this.serializedPlanOpt == other.serializedPlanOpt
case _ =>
false
}
}
- override def hashCode(): Int = Objects.hashCode(output, generator,
generatorOutput, child)
+ override def hashCode(): Int =
+ Objects.hashCode(output, generator, generatorOutput, Boolean.box(outer),
child)
Review Comment:
Small consistency point on the boxing, and the same applies to
`Boolean.box(isNullAwareAntiJoin)` further down. This file ascribes the type
rather than calling `box`, as in `Objects.hashCode(output, limit:
java.lang.Integer, child)` at line 1308, and `CometSampleExec` and
`CometWindowGroupLimitExec` do the same for `Double` and `Int`. Would `outer:
java.lang.Boolean` read better here?
--
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]
