uros-b commented on code in PR #57602:
URL: https://github.com/apache/spark/pull/57602#discussion_r3667467798
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameWindowFunctionsSuite.scala:
##########
@@ -1682,6 +1682,39 @@ class DataFrameWindowFunctionsSuite extends
SharedSparkSession
}
}
+ test("SPARK-58404: bypass partial WindowGroupLimit") {
Review Comment:
The test exercises only `row_number()`, but the PR description says the
bypass applies to all three rank-like functions (row_number, rank, dense_rank),
yet rank() and dense_rank() are not tested with
bypassPartialWindowGroupLimit=true. Please add checks for rank() and
dense_rank() too, this will also guard against future regressions.
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameWindowFunctionsSuite.scala:
##########
@@ -1682,6 +1682,39 @@ class DataFrameWindowFunctionsSuite extends
SharedSparkSession
}
}
+ test("SPARK-58404: bypass partial WindowGroupLimit") {
+ val df = Seq(
+ ("a", 0, "c"),
+ ("a", 1, "x"),
+ ("a", 2, "y"),
+ ("b", 1, "h"),
+ ("b", 1, "n"),
+ ("c", 1, "z"),
+ ("c", 2, "a")).toDF("key", "value", "order")
+
+ val window = Window.partitionBy($"key").orderBy($"order")
+ val expected = Seq(
+ Row("a", 0, "c", 1),
+ Row("b", 1, "h", 1),
+ Row("c", 2, "a", 1))
+
+ Seq(true, false).foreach { bypass =>
+ withSQLConf(
+ SQLConf.BYPASS_PARTIAL_WINDOW_GROUP_LIMIT.key -> bypass.toString,
+ SQLConf.WINDOW_GROUP_LIMIT_THRESHOLD.key -> "100") {
+ val result = df.withColumn("rn",
row_number().over(window)).where($"rn" === 1)
+ checkAnswer(result, expected)
+
+ val limits = collect(result.queryExecution.executedPlan) {
+ case w: WindowGroupLimitExec => w
+ }
+ // When bypassed, only the final WindowGroupLimit remains; otherwise
both partial and
+ // final are present since a shuffle is required.
+ assert(limits.size === (if (bypass) 1 else 2))
+ }
+ }
+ }
Review Comment:
Also, the test covers only a non-empty partitionSpec. When partitionSpec is
empty the Final node requires AllTuples distribution; bypassing partial is
arguably more beneficial here (the partial pass cannot reduce cardinality
across partitions, so it is pure overhead). A test asserting that
bypassPartialWindowGroupLimit=true with an unpartitioned window produces a
single WindowGroupLimitExec and correct results would make this boundary
explicit.
--
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]
