peter-toth commented on code in PR #58523:
URL: https://github.com/apache/spark/pull/58523#discussion_r3936025251
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2ScanExecBase.scala:
##########
@@ -92,8 +92,7 @@ trait DataSourceV2ScanExecBase
keyGroupedPartitioning match {
Review Comment:
Agreed, and we arrived at the same place from the other direction while
measuring this PR, so it is already filed as SPARK-59252
([#58529](https://github.com/apache/spark/pull/58529), merged) and in progress.
Measured by instrumenting the body and running
`KeyGroupedPartitioningSuite`: 33,051 executions as a `def` against 1,262 as a
`lazy val`. The same run shows 19,105 of those reads arriving through
`PartitioningPreservingUnaryExecNode.outputPartitioning`, itself a `def` doing
more work per call, so the ticket covers the three nodes together rather than
the scan alone.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2ScanExecBase.scala:
##########
@@ -92,8 +92,7 @@ trait DataSourceV2ScanExecBase
keyGroupedPartitioning match {
case Some(exprs) if conf.v2BucketingEnabled &&
KeyedPartitioning.supportsExpressions(exprs) &&
inputPartitions.nonEmpty &&
inputPartitions.forall(_.isInstanceOf[HasPartitionKey]) =>
- val dataTypes = exprs.map(_.dataType)
- val rowOrdering = RowOrdering.createNaturalAscendingOrdering(dataTypes)
+ val rowOrdering =
KeyedPartitioning.groupedKeyRowOrdering(exprs.map(_.dataType))
val partitionKeys =
inputPartitions.map(_.asInstanceOf[HasPartitionKey].partitionKey()).sorted(rowOrdering)
KeyedPartitioning(exprs, partitionKeys)
Review Comment:
Right, and this is now fixed in
[#58552](https://github.com/apache/spark/pull/58552), where the shape it needs
exists.
`InternalRowComparableWrapper`'s factory answers for the schema it settled
on there, both the type list and the ordering, and `KeyedPartitioning.apply`
takes a `sortKeys` flag that sorts with it. So this call site passes `sortKeys
= true` and derives neither: one factory answers for the types, the ordering
and the wrappers.
Leaving the line as it is here rather than adding an overload, since that
overload would come straight back out.
--
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]
