sunchao commented on code in PR #59996:
URL: https://github.com/apache/spark/pull/59996#discussion_r4437970308
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/objects.scala:
##########
@@ -232,12 +242,13 @@ object ObjectSerializerPruning extends Rule[LogicalPlan] {
}
// Builds new projection.
- val projectionOverSchema = ProjectionOverSchema(prunedSchema,
AttributeSet(s.output))
- val newProjects = p.projectList.map(_.transformDown {
+ val serialized = SerializeFromObject(nestedPrunedSerializer, s.child)
+ val projectionOverSchema = ProjectionOverSchema(serialized.schema,
AttributeSet(s.output))
Review Comment:
I reproduced the double visit on both master and this branch. Your
single-visit change prunes the Map value and the sibling struct and still
returns `[map-b, other-a]`. I’d keep that pruning improvement as a follow-up so
this bug fix can be backported independently. In 3a5d7b78fd2 the regression
test now checks that it actually exercises the full-serializer fallback. I also
updated the description to distinguish the executor-side array exception from
the struct case that fails during planning.
##########
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:
Updated in 3a5d7b78fd2. The comment now explains that `items` and `ITEMS`
can otherwise become distinct root fields during case-insensitive pruning.
##########
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:
Changed in 3a5d7b78fd2. The disabled branch now returns after top-level
serializer pruning. The enabled branch uses plain vals and transforms
`p.projectList` directly. I checked the result values and output names with the
setting both on and off.
##########
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:
Added that assertion in 3a5d7b78fd2. With the current fallback, the test
checks the emitted schema and the returned values. I tried both mutations:
using the requested schema gives `map-a` and fails the value check, while
visiting each Map converter once changes the emitted schema and fails the new
assertion. The description now also explains why separate top-level RowEncoder
columns prune normally.
--
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]
