ulysses-you commented on code in PR #57742:
URL: https://github.com/apache/spark/pull/57742#discussion_r3720253668
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -4156,6 +4156,46 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val ADAPTIVE_PARTIAL_AGGREGATION_ENABLED =
+
buildConf("spark.sql.execution.aggregate.adaptivePartialAggregation.enabled")
+ .doc("When true, hash aggregation adaptively bypasses the pre-shuffle
partial aggregation " +
+ "at runtime when it observes that the partial aggregation is not
reducing the number of " +
+ "rows enough to be worthwhile. Once bypassed, the remaining input rows
are passed " +
+ "through as single-row partial aggregation buffers for the final
aggregation to merge, " +
+ "which avoids the cost of maintaining and spilling a large aggregation
map with little " +
+ "reduction benefit. This applies only to hash aggregation with
grouping keys.")
+ .version("4.4.0")
+ .withBindingPolicy(ConfigBindingPolicy.SESSION)
+ .booleanConf
+ .createWithDefault(true)
+
+ val ADAPTIVE_PARTIAL_AGGREGATION_MIN_ROWS =
+
buildConf("spark.sql.execution.aggregate.adaptivePartialAggregation.minRows")
+ .doc("The number of rows to process before adaptive partial aggregation
(see " +
+ s"'${ADAPTIVE_PARTIAL_AGGREGATION_ENABLED.key}') evaluates the
compaction ratio. The " +
+ "ratio is evaluated once this many rows have been processed since the
previous " +
+ "evaluation, so a decision is never made on too few rows.")
+ .version("4.4.0")
+ .withBindingPolicy(ConfigBindingPolicy.SESSION)
+ .longConf
+ .checkValue(_ > 0, "The minimum row count must be positive.")
Review Comment:
Done in d95d6c52655 -- the validator is now `_ >= 0`.
No execution-path change was needed: `nextCheckRow` starts at `minRows`, and
the count is only compared after being incremented past 0, so with `minRows =
0` the equality never holds and the periodic check simply never fires. The
spill check is guarded by `processedRows > 0 && ineffective()`, independent of
`minRows`, so it keeps working. I verified this rather than assuming it, and
documented the sentinel in the config doc, `sql-performance-tuning.md`, and at
both read sites so a later refactor does not silently break it.
Two tests, since the spill-only one alone would not prove the periodic check
is off:
- `minRows = 0` with no forced spill on fully distinct input -- asserts
nothing bypasses. The same query with `minRows = 8` is asserted to bypass
elsewhere in the suite, so this is a real signal.
- `minRows = 0` with a forced regular-map spill -- asserts `numBypassingRows
> 0` and `numTasksFallBacked == 0`, i.e. the spill-only mode you described.
--
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]
