peter-toth commented on code in PR #59199:
URL: https://github.com/apache/spark/pull/59199#discussion_r4207877792
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanPartitioningAndOrdering.scala:
##########
@@ -104,8 +104,12 @@ object V2ScanPartitioningAndOrdering extends
Rule[LogicalPlan] with Logging {
// scan output: truncating it here would also drop the sort orders on
a partition key past a
// pruned column, which still hold.
`DataSourceV2ScanRelation.doCanonicalize` and
// `DataSourceV2ScanExecBase.outputOrdering` restrict it to the scan
output instead.
+ // A nested sort key such as `s.x` resolves to a new
`Alias(GetStructField(...))` on every
+ // run of this rule. Strip the alias so repeated runs give equal plans
and the ordering
+ // can satisfy a required ordering on the same field.
val ordering =
V2ExpressionUtils.toCatalystOrdering(reportedOrdering, relation,
relation.funCatalog)
+ .map(o => o.copy(child = trimAliases(o.child)))
Review Comment:
**Finding 2.** The description's third "why" bullet (`PlanMerger` treats two
derivations of the same nested ordering as different) is the one effect without
a test. I measured it at this head with a `MergeSubplansSuite` case beside
"SPARK-58549: merge DSv2 scans when the rebuilt scan re-derives the required
ordering":
- a `TestV2Table` with `s struct<x int>, b int, c string` and
`reportedOrderingCols = Seq("s.x")`;
- two `ScalarSubquery` sums, over `v2ScanReportingOn(table, Seq("s", "b"))`
and over `v2ScanReportingOn(table, Seq("s", "c"))`;
- `assert(v2Scans(Optimize.execute(q.analyze)).length == 1)`.
With this file at the merge base it fails with "had length 2 instead of
expected length 1", since the ordering prints as `s#7.x AS x#11`. At this head
it passes. Could we add it?
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanPartitioningAndOrdering.scala:
##########
@@ -104,8 +104,12 @@ object V2ScanPartitioningAndOrdering extends
Rule[LogicalPlan] with Logging {
// scan output: truncating it here would also drop the sort orders on
a partition key past a
// pruned column, which still hold.
`DataSourceV2ScanRelation.doCanonicalize` and
// `DataSourceV2ScanExecBase.outputOrdering` restrict it to the scan
output instead.
+ // A nested sort key such as `s.x` resolves to a new
`Alias(GetStructField(...))` on every
+ // run of this rule. Strip the alias so repeated runs give equal plans
and the ordering
+ // can satisfy a required ordering on the same field.
Review Comment:
**Finding 3.** This holds while the scan reads the whole struct. Once nested
schema pruning narrows `s`, the required ordering's `s` has the pruned type and
this one the full type, so they still differ (SPARK-60011). Could the comment
say "when the scan reads the whole struct, see SPARK-60011", and the
user-facing section of the description too? That section could also mention
that DSv2 scans reporting a nested ordering can now be merged.
--
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]
