peter-toth commented on code in PR #57742:
URL: https://github.com/apache/spark/pull/57742#discussion_r3755080983
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/TungstenAggregationIterator.scala:
##########
@@ -191,29 +209,67 @@ class TungstenAggregationIterator(
}
} else {
var i = 0
- while (inputIter.hasNext) {
+ var processedRows = 0L
+ val minRows = adaptiveMinRows
+ // The processed-row count at which the compaction ratio is evaluated
next. It advances by
+ // `minRows` after every check, and restarts after a spill so the new
in-memory map epoch is
+ // judged on its own rows. `minRows = 0` disables the periodic check:
the count is only ever
+ // compared after being incremented past 0, so it never matches and only
the spill check
+ // below remains.
+ var nextCheckRow = minRows
+ // The partial aggregation is ineffective when it does not collapse
`minCompaction` rows into
+ // one key. There is no fast map on this path, so the map's keys are all
the operator holds.
+ def ineffective(): Boolean =
+ processedRows < hashMap.getNumKeys().toDouble * adaptiveMinCompaction
+ while (inputIter.hasNext && !passThrough) {
Review Comment:
**Finding 15.** Correcting what I claimed for this at R2, now that I've
traced both findings it touches.
It does fully settle finding 1. If the map keeps being probed, a key that's
present absorbs all of its rows in place and a key that's absent streams all of
them, so no group ever has buffers racing each other to the `Final` and the
per-partition merge order matches a non-bypassed run exactly. Freezing is what
makes the order unstable in the first place, and finding 1's new repro is that
instability surfacing in a shape the new order test can't reach.
It does not settle finding 2, which I said it would. It only helps when the
collapsing tail reuses keys the prefix already inserted; a tail whose hot keys
are new isn't in the frozen map, so every one of its rows still streams.
Partial mitigation there.
Cost, from your regenerated numbers: roughly 1.4 ns/row of probe when the
aggregation is worth keeping, against the ~204 ns/row of insert-and-grow work
the freeze saves when it isn't. Whether that trade is worth taking is your call
— I'm raising it mainly for the order property.
--
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]
