viirya commented on code in PR #59996:
URL: https://github.com/apache/spark/pull/59996#discussion_r4437932629
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/objects.scala:
##########
@@ -221,7 +222,16 @@ object ObjectSerializerPruning extends Rule[LogicalPlan] {
val usedRefs = p.references
val prunedSerializer = s.serializer.filter(usedRefs.contains)
- val rootFields = SchemaPruning.identifyRootFields(p.projectList,
Seq.empty)
+ // Resolved references can use the spelling from the query. Both pruning
and rebuilding
+ // field accesses need the names from the serializer, including when a
root is used twice.
Review Comment:
Could the comment give the concrete case? Something like "otherwise `items`
and `ITEMS` become two distinct root fields in `pruneSchema`". "When a root is
used twice" is hard to follow without it.
##########
sql/core/src/test/scala/org/apache/spark/sql/DatasetOptimizationSuite.scala:
##########
@@ -132,6 +132,25 @@ class DatasetOptimizationSuite extends SharedSparkSession {
}
}
+ test("Nested projections bind to the fields emitted by the serializer") {
+ val ds = spark.range(1).map { _ =>
+ Tuple1((Map("k" -> ("map-a", "map-b")), ("other-a", "other-b")))
+ }
+ // Projecting the sibling as well can leave the original serializer in
place.
Review Comment:
This test reaches the fallback only through the `ExternalMapToCatalyst`
double visit in `pruneSerializer`. Once that's fixed (peter-toth's Finding 1),
the test will still pass but no longer cover the `serialized.schema` change.
Could you assert here that the serializer is left unpruned, and say why in the
comment, so a follow-up has to revisit this test?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/objects.scala:
##########
@@ -221,7 +222,16 @@ object ObjectSerializerPruning extends Rule[LogicalPlan] {
val usedRefs = p.references
val prunedSerializer = s.serializer.filter(usedRefs.contains)
- val rootFields = SchemaPruning.identifyRootFields(p.projectList,
Seq.empty)
+ // Resolved references can use the spelling from the query. Both pruning
and rebuilding
+ // field accesses need the names from the serializer, including when a
root is used twice.
+ lazy val normalizedProjects = {
+ val namesById = s.output.map(a => a.exprId -> a.name).toMap
+ p.transformExpressions {
+ case a: AttributeReference if namesById.contains(a.exprId) =>
+ a.withName(namesById(a.exprId))
+ }.projectList
+ }
+ lazy val rootFields =
SchemaPruning.identifyRootFields(normalizedProjects, Seq.empty)
Review Comment:
Nit: the two `lazy val`s are only there for the `&&` short-circuit. An
explicit `if (!conf.serializerNestedSchemaPruningEnabled) ... else { ... }`
would make them plain `val`s. Also, `p.projectList.map(_.transform { ... })`,
as in `SchemaPruning.normalizeAttributeRefNames`, reads more directly than
going through `p.transformExpressions { ... }.projectList`.
--
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]
