[SPARK-54272][SQL] Add aggTime for SortAggregateExec #52968
Conversation
AngersZhuuuu
changed the title
|
@HyukjinKwon Could you help review metrics related. |
|
cc @cloud-fan |
| outputIter | ||
| } | ||
| } | ||
| aggTime += NANOSECONDS.toMillis(System.nanoTime() - beforeAgg) |
There was a problem hiding this comment.
is it the right level to trace the agg time? I think the iterator is lazy, no?
There was a problem hiding this comment.
Also find. it's a little strange...If so HashAggregateExec also incorrect?
There was a problem hiding this comment.
So I think the difference is the TungstenAggregationIterator is not as lazy -- during it's init step it does the aggregation and whereas sortbasedaggregationiterator does the compute mostly inside of next
There was a problem hiding this comment.
Got it, missing TungstenAggregationIterator will call processInputs during construction. So how about my current change?
There was a problem hiding this comment.
ping @cloud-fan @holdenk @dongjoon-hyun Could you take a look
|
This looks reasonable to me, I'd love @cloud-fan to sign-off though :) |
| test("SortAggregate metrics") { | ||
| // Force use SortAggregateExec instead of HashAggregateExec | ||
| withSQLConf("spark.sql.test.forceApplySortAggregate" -> "true", | ||
| SQLConf.WHOLESTAGE_CODEGEN_ENABLED.key -> "false") { |
There was a problem hiding this comment.
nit: other tests in this suite do not turn off whole stage codegen, why it's necessary here?
There was a problem hiding this comment.
Since SortAggregateExec not support codegen then write this, remove it is ok too. remove this line.
|
thanks, merging to master! |
4 participants
What changes were proposed in this pull request?
Add
aggTimemetrics forSortAggregateExecWhy are the changes needed?
Add more metrics
Does this PR introduce any user-facing change?
Yes the SQL metrics "time in aggregation build" itself on Spark UI.
How was this patch tested?
UT
Was this patch authored or co-authored using generative AI tooling?
No