dongjoon-hyun commented on code in PR #58543:
URL: https://github.com/apache/spark/pull/58543#discussion_r3952269049
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/GroupPartitionsExec.scala:
##########
@@ -440,8 +451,11 @@ case class GroupPartitionsExec(
s"Reducers: ${truncatedString(names, "[", ", ", "]", joinKeyMaxFields)}"
}
val distributeStr = Iterator(s"DistributePartitions:
$distributePartitions")
- joinKeyStr ++ expectedStr ++ reducersStr ++ distributeStr
-
+ // Rendered from the constructor field, as `DistributePartitions` above
is. Not from
+ // `usesSortedMerge`, because that forces `grouping`, which can throw, and
this method feeds
+ // `simpleString`, which `treeString` calls on error paths.
+ val sortedMergeStr = Iterator(s"SortedMerge: $enableSortedMerge")
Review Comment:
Just noting for the record: this adds `SortedMerge: false` to every
`GroupPartitions` line in EXPLAIN, even for nodes that do not coalesce. It is
consistent with `DistributePartitions`, and no golden file contains
`GroupPartitions`, so nothing else breaks. But since this will be backported to
`branch-4.3` and `branch-4.2`, the EXPLAIN string changes in maintenance
releases too. I am fine with it, just want it to be a conscious choice.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/GroupPartitionsExecSuite.scala:
##########
@@ -411,22 +409,37 @@ class GroupPartitionsExecSuite extends SharedSparkSession
{
assert(!GroupPartitionsExec(child).groupedPartitions.forall(_._2.size <=
1),
"expected coalescing")
- withSQLConf(SQLConf.V2_BUCKETING_PRESERVE_ORDERING_ON_COALESCE_ENABLED.key
-> "true") {
- assert(GroupPartitionsExec(child).outputOrdering !== childOrdering,
- "config alone should not enable k-way merge; enableSortedMerge must be
set by planner")
- assert(GroupPartitionsExec(child, enableSortedMerge =
true).outputOrdering === childOrdering)
- }
- withSQLConf(
- SQLConf.V2_BUCKETING_PRESERVE_ORDERING_ON_COALESCE_ENABLED.key ->
"false",
- SQLConf.V2_BUCKETING_PRESERVE_KEY_ORDERING_ON_COALESCE_ENABLED.key ->
"true") {
- // Sorted-merge config disabled, key-ordering config enabled: only
key-expression orders
- // survive simple concatenation (non-key exprC is dropped).
- val ordering = GroupPartitionsExec(child, enableSortedMerge =
true).outputOrdering
+ assert(GroupPartitionsExec(child, enableSortedMerge = true).outputOrdering
=== childOrdering)
+
withSQLConf(SQLConf.V2_BUCKETING_PRESERVE_KEY_ORDERING_ON_COALESCE_ENABLED.key
-> "true") {
+ // Without the flag there is no k-way merge, so only key-expression
orders survive simple
+ // concatenation and the non-key exprC is dropped.
+ val ordering = GroupPartitionsExec(child).outputOrdering
assert(ordering.length === 1)
assert(ordering.head.child === exprA)
}
}
+ test("SPARK-59279: enableSortedMerge decides the k-way merge, not the
config") {
+ // The config is the planner's input, and `enableSortedMerge` records what
the planner decided
+ // under it. Once the flag is set the config no longer matters, because
the plan above was
+ // built on the ordering the merge delivers.
+ val partitionKeys = Seq(row(1), row(2), row(1))
+ val childOrdering = Seq(SortOrder(exprA, Ascending), SortOrder(exprC,
Ascending))
+ val child = DummyLeafSparkPlan(
+ outputPartitioning = KeyedPartitioning(Seq(exprA), partitionKeys),
+ outputOrdering = childOrdering)
+
+ Seq(true, false).foreach { configEnabled =>
Review Comment:
The PR description says this is "a grid over the config with and without the
flag", but the test only iterates the config with `enableSortedMerge = true`.
The assertion this PR removed from the test above ("config alone should not
enable k-way merge") is no longer checked for `config = true`. How about adding
the `enableSortedMerge = false` case here, asserting that
`GroupPartitionsExec(child).outputOrdering !== childOrdering` regardless of the
config? Then the test states exactly what its name says.
--
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]
