comphead commented on code in PR #6986:
URL: https://github.com/apache/datafusion-comet/pull/6986#discussion_r4274438474
##########
spark/src/test/scala/org/apache/comet/serde/QueryPlanSerdeSuite.scala:
##########
@@ -121,4 +122,42 @@ class QueryPlanSerdeSuite extends AnyFunSuite {
assert(!canShortCircuitNulls(position))
}
}
+
+ test("canShortCircuitNulls follows subexpression elimination except in
aggregates") {
+ val cast = Cast(AttributeReference("s", StringType)(), IntegerType)
+ val array = AttributeReference("ai", ArrayType(IntegerType))()
+ val contains = ArrayContains(array, cast)
+ val shared = Seq(contains, ArrayPosition(array, cast))
+
+ def withSubexpressionElimination(enabled: Boolean)(f: => Unit): Unit = {
+ val conf = new SQLConf
+ conf.setConf(SQLConf.SUBEXPRESSION_ELIMINATION_ENABLED, enabled)
+ SQLConf.withExistingConf(conf) {
+ assert(SQLConf.get.subexpressionEliminationEnabled == enabled)
+ f
+ }
+ }
+
+ withSubexpressionElimination(enabled = false) {
+ withSharedSubexpressions(shared) {
+ assert(canShortCircuitNulls(contains))
+ }
+ withSharedSubexpressions(shared, alwaysShared = true) {
+ assert(!canShortCircuitNulls(contains))
+ }
+ // Outside an operator's conversion what they share is still unknown
+ assert(!canShortCircuitNulls(contains))
+ }
+ withSubexpressionElimination(enabled = true) {
+ Seq(false, true).foreach { alwaysShared =>
+ withSharedSubexpressions(shared, alwaysShared) {
+ assert(!canShortCircuitNulls(contains))
+ }
+ withSharedSubexpressions(Seq(contains), alwaysShared) {
+ assert(canShortCircuitNulls(contains))
+ }
+ }
+ assert(!canShortCircuitNulls(contains))
+ }
Review Comment:
With the setting on, `SharedSubexpressions.common` is `alwaysShared ||
enabled`, so looping `alwaysShared` over `Seq(false, true)` adds no case for
`true`. The `shared` assertion and the `Seq(contains)` assertion also repeat
what `canShortCircuitNulls skips an argument only where the shared
subexpressions are known` (line 106) already checks, since the default conf has
elimination on. The new behaviour is the `enabled = false` block above. Could
this block shrink to one `alwaysShared = false` case, or go?
--
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]
