dwsmith1983 commented on code in PR #6986:
URL: https://github.com/apache/datafusion-comet/pull/6986#discussion_r4278199163
##########
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:
> Could this block shrink to one `alwaysShared = false` case, or go?
It went. With elimination on, `alwaysShared` doesn't change the shared set,
and the remaining assertions (a shared cast blocks the skip, the cast alone
allows it, unknown outside a scope) are the ones `canShortCircuitNulls skips an
argument only where the shared subexpressions are known` already makes under
the default conf. The new test keeps only the `enabled = false` block, and it
still fails if `SharedSubexpressions.common` ignores the setting.
--
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]
