cloud-fan commented on code in PR #57153:
URL: https://github.com/apache/spark/pull/57153#discussion_r3566868206
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/WholeStageCodegenSuite.scala:
##########
@@ -68,6 +68,106 @@ class WholeStageCodegenSuite extends SharedSparkSession
}
}
+ // Runs `query` on `data` with sort aggregate forced and its code-gen
enabled, asserts the plan
+ // actually uses a code-gen'd SortAggregateExec, and checks the result
matches the interpreted
+ // (code-gen disabled) result.
+ private def checkSortAggregateCodegen(
+ data: Dataset[Row])(query: Dataset[Row] => Dataset[Row]): Unit = {
+ // Disable both hash-based aggregate operators so the planner always picks
SortAggregateExec.
+ val forceSortAggregate = Seq(
+ SQLConf.USE_HASH_AGG.key -> "false",
+ SQLConf.USE_OBJECT_HASH_AGG.key -> "false")
+ val expected = withSQLConf(
+ (forceSortAggregate :+ (SQLConf.ENABLE_SORT_AGGREGATE_CODEGEN.key ->
"false")): _*) {
+ val df = query(data)
+ assert(!df.queryExecution.executedPlan.exists(p =>
+ p.isInstanceOf[WholeStageCodegenExec] &&
+
p.asInstanceOf[WholeStageCodegenExec].child.isInstanceOf[SortAggregateExec]),
+ s"Expected a code-gen'd SortAggregateExec
in:\n${df.queryExecution.executedPlan}")
+ df.collect()
+ }
+ withSQLConf(
+ (forceSortAggregate :+ (SQLConf.ENABLE_SORT_AGGREGATE_CODEGEN.key ->
"true")): _*) {
+ val df = query(data)
+ assert(df.queryExecution.executedPlan.exists(p =>
+ p.isInstanceOf[WholeStageCodegenExec] &&
+
p.asInstanceOf[WholeStageCodegenExec].child.isInstanceOf[SortAggregateExec]),
+ s"Expected a code-gen'd SortAggregateExec
in:\n${df.queryExecution.executedPlan}")
+ checkAnswer(df, expected)
+ }
+ }
+
+ test("SPARK-32750: SortAggregate code-gen with grouping keys") {
+ val data = spark.range(200).selectExpr(
+ "id",
+ "id % 7 as k1",
+ "id % 3 as k2",
+ "case when id % 5 = 0 then null else id end as v",
+ "case when id % 4 = 0 then null else cast(id % 11 as string) end as s")
Review Comment:
The PR description claims the new tests cover "decimal grouping keys",
"split aggregate functions", and "the config gate", but the diff doesn't back
this:
- No new test uses a decimal (or float) grouping key — the test columns here
are long `k1`/`k2` and string `s`; decimal only appears in the benchmark.
- No new test sets `CODEGEN_METHOD_SPLIT_THRESHOLD` /
`CODEGEN_SPLIT_AGGREGATE_FUNC`, so with the default threshold (1024) these
small queries never reach the split-aggregate path.
- No new test references `ENABLE_SORT_AGGREGATE_CODEGEN_WITH_KEYS`, so the
new config gate is never toggled.
Either add the coverage or correct the description. (The decimal half
overlaps @LuciferYang's thread just below on float/`-0.0`/`NaN` correctness,
which is the sharper concern.)
--
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]
