LuciferYang commented on code in PR #13198:
URL: https://github.com/apache/gluten/pull/13198#discussion_r4196789644
##########
gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala:
##########
@@ -977,31 +975,34 @@ object ExpressionConverter extends SQLConfHelper with
Logging {
if (!root.isInstanceOf[AttributeReference]) {
return BoundReference(structField.ordinal, structField.dataType,
structField.nullable)
}
- names += root.asInstanceOf[AttributeReference].name
- input.attrs.foreach(
- attribute => {
- var level = names.size - 1
- if (names(level) == attribute.name) {
- var candidateFields: Array[StructField] = null
- var dtType = attribute.dataType
- while (dtType.isInstanceOf[StructType] && level >= 1) {
- candidateFields = dtType.asInstanceOf[StructType].fields
- level -= 1
- val curName = names(level)
- for (i <- 0 until candidateFields.length) {
- if (candidateFields(i).name == curName) {
- dtType = candidateFields(i).dataType
- newOrdinal = i
- }
- }
- }
- }
- })
- if (newOrdinal == -1) {
+ def cannotBind(reason: String): Nothing =
throw new IllegalStateException(
- s"Couldn't find $structField in ${input.attrs.mkString("[", ",",
"]")}")
- } else {
- BoundReference(newOrdinal, structField.dataType, structField.nullable)
+ s"Couldn't bind $structField ($reason) in ${input.attrs.mkString("[",
",", "]")}")
+ // Bind the root by exprId like BindReferences: the input can hold several
same-named struct
+ // attributes with different layouts, e.g. a struct column of the same
name on both join sides.
+ val rootAttr = root.asInstanceOf[AttributeReference]
+ val rootIndex = input.indexOf(rootAttr.exprId)
+ if (rootIndex < 0) {
+ cannotBind(s"no input attribute with the exprId of ${rootAttr.name}")
+ }
+ val attribute = input.attrs(rootIndex)
+ // Same layout as the one the expression was resolved against, so its own
ordinal is right.
+ // It is also the only right answer when the struct has duplicate field
names.
+ if (attribute.dataType == rootAttr.dataType) {
+ return BoundReference(structField.ordinal, structField.dataType,
structField.nullable)
+ }
+ // The input struct type differs (e.g. nested nullability merged by a
union): look each level
+ // up by name from the root down, and fall back instead of guessing on a
duplicate name.
+ val (_, ordinal) = names.reverseIterator.foldLeft[(DataType,
Int)]((attribute.dataType, -1)) {
+ case ((struct: StructType, _), name) =>
+ struct.fields.indexWhere(_.name == name) match {
+ case -1 => cannotBind(s"no field $name")
+ case idx if struct.fields.lastIndexWhere(_.name == name) != idx =>
+ cannotBind(s"ambiguous field $name")
Review Comment:
Yes. The `GetStructField` arm in `replaceWithExpressionTransformer0` catches
the `IllegalStateException` and rethrows it as `UnsupportedOperationException`,
and `ValidatablePlan.failValidationWithException` turns that into a failed
validation, so the projection falls back. Added an end-to-end case in
47894efde: a struct with two fields named `a` read through a union with an
empty branch. After the empty branch is removed, the projection sees the same
exprId with a different struct type, takes the by-name lookup, hits the
ambiguous `a`, and falls back (reason: `ambiguous field a`). The test asserts
the read runs in `ProjectExec` and matches vanilla Spark; it fails on main.
--
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]
