viirya commented on code in PR #59192:
URL: https://github.com/apache/spark/pull/59192#discussion_r4170052049
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasing.scala:
##########
@@ -275,15 +275,15 @@ object NestedColumnAliasing {
// Remove redundant [[ExtractValue]]s if they share the same parent
nest field.
// For example, when `a.b` and `a.b.c` are in project list, we only
need to alias `a.b`.
// Because `a.b` requires all of the inner fields of `b`, we cannot
prune `a.b.c`.
- val dedupNestedFields = nestedFields.filter {
+ val distinctNestedFields = nestedFields.distinct
+ val nestedFieldSet = ExpressionSet(distinctNestedFields)
+ val dedupNestedFields = distinctNestedFields.filter {
// See [[collectExtractValue]]: we only need to deal with
[[GetArrayStructFields]] and
// [[GetStructField]]
case e @ (_: GetStructField | _: GetArrayStructFields) =>
- val child = e.children.head
- nestedFields.forall(f => !child.exists(_.semanticEquals(f)))
+ !e.children.head.exists(nestedFieldSet.contains)
Review Comment:
Could we retain the query-side determinism check explicitly?
```scala
!e.children.head.exists(p => p.deterministic && nestedFieldSet.contains(p))
```
`semanticEquals` checks that both expressions are deterministic.
`ExpressionSet.add` excludes nondeterministic entries from its lookup set, but
`contains` only checks the canonicalized expression.
I have not found a reachable counterexample in the normal optimizer flow,
but retaining this guard would preserve the original condition explicitly and
make the behavior-preserving argument easier to verify.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasingSuite.scala:
##########
@@ -333,6 +333,45 @@ class NestedColumnAliasingSuite extends SchemaPruningTest {
comparePlans(optimized, expected)
}
+ test("Redundant nested fields are detected through cosmetic variations of
their parents") {
+ val a = $"a".struct(StructType.fromDDL("b struct<c: int, d: int>, e int"))
+ // `a.b`, referenced many times below.
+ val ab = GetStructField(a, 0, Some("b"))
+ // `a.b` without the field name.
+ val unnamedAb = GetStructField(a, 0, None)
+ // `a.b.c` over the unnamed `a.b`.
+ val abc = GetStructField(unnamedAb, 0, Some("c"))
+ // `a.b.d` over a qualified `a`.
+ val abd = GetStructField(
+ GetStructField(a.withQualifier(Seq("t")), 0, Some("b")), 1, Some("d"))
+
+ val attrToExtractValues = NestedColumnAliasing.getAttributeToExtractValues(
+ Seq.fill(100)(ab) ++ Seq(unnamedAb, abc, abd), Seq.empty)
+
+ assert(attrToExtractValues.keys.map(_.exprId).toSeq == Seq(a.exprId))
+ assert(attrToExtractValues.values.toSeq == Seq(Seq(ab, unnamedAb)))
+ }
+
+ test("Nested fields are not redundant with non-deterministic parents") {
+ val a = $"a".array(StructType.fromDDL("b struct<c: int, d: int>, e int, f
int"))
+ // `a[ordinal].b` and `a[ordinal].b.c`.
+ def parentAndChild(ordinal: Expression): Seq[ExtractValue] = {
+ val parent = GetStructField(GetArrayItem(a, ordinal), 0, Some("b"))
+ Seq(parent, GetStructField(parent, 0, Some("c")))
+ }
+
+ val deterministic = parentAndChild(Literal(0))
+ assert(NestedColumnAliasing.getAttributeToExtractValues(deterministic,
Seq.empty)
+ .values.toSeq == Seq(deterministic.take(1)))
+
+ // Each evaluation of `a[rand].b` may read a different array element, so
`a[rand].b.c` is
+ // not redundant with it.
+ val nondeterministic =
+ parentAndChild(Cast(Multiply(Rand(Literal(0L)), Literal(2.0)),
IntegerType))
Review Comment:
Could we also test deterministic and nondeterministic reads together in the
same call, under the same root attribute?
Here, both candidates are nondeterministic, so `ExpressionSet`'s internal
lookup set is empty. A mixed case would exercise a populated set and verify
that the deterministic child is removed while the nondeterministic parent and
child remain. The schema would need enough unused fields to keep aliasing
eligible.
--
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]
