You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2024/07/29 13:42:25 UTC
[PR] Add metrics for skipped rows [datafusion]
alamb opened a new pull request, #11706:
URL: https://github.com/apache/datafusion/pull/11706
Builds on https://github.com/apache/datafusion/pull/11695
## Which issue does this PR close?
Part of https://github.com/apache/datafusion/issues/6937
## Rationale for this change
https://github.com/apache/datafusion/pull/11627 from @korowa adds a "partial aggregation skipping" mode to the hash aggregate exec that applies in some, but not all cases
It would be very nice to know if the path is being executed or not
## What changes are included in this PR?
Add a "skipped_aggregation_rows" counter to the
## Are these changes tested?
I tested them manually
For example, this line shows ` skipped_aggregation_rows=98293561`:
```
| | AggregateExec: mode=Partial, gby=[WatchID@0 as WatchID, ClientIP@1 as ClientIP], aggr=[count(*)], metrics=[output_rows=99997497, elapsed_compute=190.246833ms, skipped_aggregation_rows=98293561]
|
```
Here is the entire output of testing with a query:
```
$ ./datafusion-cli-skip-partial-metrics -c "EXPLAIN ANALYZE SELECT \"WatchID\", \"ClientIP\", COUNT(*) AS c FROM 'hits.parquet' GROUP BY \"WatchID\", \"ClientIP\" ORDER BY c DESC LIMIT 10;"
DataFusion CLI v40.0.0
+-------------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------+
| plan_type | plan
|
+-------------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------+
| Plan with Metrics | GlobalLimitExec: skip=0, fetch=10, metrics=[output_rows=10, elapsed_compute=14.916µs]
|
| | SortPreservingMergeExec: [c@2 DESC], fetch=10, metrics=[output_rows=10, elapsed_compute=3.667µs]
|
| | SortExec: TopK(fetch=10), expr=[c@2 DESC], preserve_partitioning=[true], metrics=[output_rows=160, elapsed_compute=764.776988ms, row_replacements=164]
|
| | ProjectionExec: expr=[WatchID@0 as WatchID, ClientIP@1 as ClientIP, count(*)@2 as c], metrics=[output_rows=99997493, elapsed_compute=1.960225ms]
|
| | AggregateExec: mode=FinalPartitioned, gby=[WatchID@0 as WatchID, ClientIP@1 as ClientIP], aggr=[count(*)], metrics=[output_rows=99997493, elapsed_compute=17.934170583s]
|
| | CoalesceBatchesExec: target_batch_size=8192, metrics=[output_rows=99997497, elapsed_compute=598.045136ms]
|
| | RepartitionExec: partitioning=Hash([WatchID@0, ClientIP@1], 16), input_partitions=16, metrics=[repart_time=1.018381539s, send_time=8.695161456s, fetch_time=1.684569996s]
|
| here--> | AggregateExec: mode=Partial, gby=[WatchID@0 as WatchID, ClientIP@1 as ClientIP], aggr=[count(*)], metrics=[output_rows=99997497, elapsed_compute=190.246833ms, skipped_aggregation_rows=98293561]
|
| | ParquetExec: file_groups={16 groups: [[Users/andrewlamb/Downloads/hits.parquet:0..923748528], [Users/andrewlamb/Downloads/hits.parquet:923748528..1847497056], [Users/andrewlamb/Downloads/hits.parquet:1847497056..2771245584], [Users/andrewlamb/Downloads/hits.parquet:2771245584..3694994112], [Users/andrewlamb/Downloads/hits.parquet:3694994112..4618742640], ...]}, projection=[WatchID, ClientIP], metrics=[output_rows=99997497, elapsed_compute=16ns, page_index_rows_filtered=0, row_groups_matched_statistics=0, row_groups_matched_bloom_filter=0, file_scan_errors=0, row_groups_pruned_bloom_filter=0, predicate_evaluation_errors=0, bytes_scanned=1050612937, pushdown_rows_filtered=0, file_open_errors=0, num_predicate_creation_errors=0, row_groups_pruned_statistics=0, pushdown_eval_time=32ns, time_elapsed_processing=978.395665ms, page_index_eval_time=32ns, time_elapsed_scanning_until_data=68.642586ms, time_elapsed_scanning_total=16.706024614s, time_elapse
d_opening=380.053666ms] |
| |
|
+-------------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------+
1 row(s) fetched.
Elapsed 1.485 seconds.
```
I will see if I can find a way to do so
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `skipped_aggregation_rows`to aggregate operator [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11706:
URL: https://github.com/apache/datafusion/pull/11706#issuecomment-2271331663
FYI @Dandandan would you have time to review this PR (to help performance profiling of the group by aggregation skipping code)?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `skipped_aggregation_rows` metric to aggregate operator [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #11706:
URL: https://github.com/apache/datafusion/pull/11706
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `skipped_aggregation_rows` metric to aggregate operator [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11706:
URL: https://github.com/apache/datafusion/pull/11706#issuecomment-2273915127
Thanks for the review @andygrove
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `skipped_aggregation_rows`to aggregate operator [datafusion]
Posted by "korowa (via GitHub)" <gi...@apache.org>.
korowa commented on code in PR #11706:
URL: https://github.com/apache/datafusion/pull/11706#discussion_r1704911308
##########
datafusion/physical-plan/src/aggregates/row_hash.rs:
##########
@@ -473,17 +491,17 @@ impl GroupedHashAggregateStream {
.all(|acc| acc.supports_convert_to_state())
&& agg_group_by.is_single()
{
+ let options = &context.session_config().options().execution;
+ let probe_rows_threshold =
+ options.skip_partial_aggregation_probe_rows_threshold;
+ let probe_ratio_threshold =
+ options.skip_partial_aggregation_probe_ratio_threshold;
+ let skipped_aggregation_rows = MetricBuilder::new(&agg.metrics)
Review Comment:
Agree that it should be better not to add this counter into all group bys (as another baseline metric) by default :+1:
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `skipped_aggregation_rows`to aggregate operator [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11706:
URL: https://github.com/apache/datafusion/pull/11706#discussion_r1704094369
##########
datafusion/physical-plan/src/aggregates/row_hash.rs:
##########
@@ -611,6 +629,9 @@ impl Stream for GroupedHashAggregateStream {
match ready!(self.input.poll_next_unpin(cx)) {
Some(Ok(batch)) => {
let _timer = elapsed_compute.timer();
+ if let Some(probe) = self.skip_aggregation_probe.as_mut() {
+ probe.record_skipped(&batch);
Review Comment:
This is the actual call that records the metrics . The rest of the PR is comments and plumbing
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org