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/08/05 11:27:20 UTC
[PR] Improve comments in row_hash.rs for skipping aggregation [datafusion]
alamb opened a new pull request, #11820:
URL: https://github.com/apache/datafusion/pull/11820
## Which issue does this PR close?
Follow on to https://github.com/apache/datafusion/pull/11627
## Rationale for this change
@2010YOUY01 had some good suggestions on https://github.com/apache/datafusion/pull/11627 such as https://github.com/apache/datafusion/pull/11627/files#r1695417159 which I wanted to capture as part of filing the follow up work
## What changes are included in this PR?
Update some comments
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
Comment changes only, no functional 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] Minor: Improve comments in row_hash.rs for skipping aggregation [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #11820:
URL: https://github.com/apache/datafusion/pull/11820
--
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] Improve comments in row_hash.rs for skipping aggregation [datafusion]
Posted by "korowa (via GitHub)" <gi...@apache.org>.
korowa commented on code in PR #11820:
URL: https://github.com/apache/datafusion/pull/11820#discussion_r1711819657
##########
datafusion/physical-plan/src/aggregates/row_hash.rs:
##########
@@ -100,22 +100,24 @@ struct SpillState {
///
/// See "partial aggregation" discussion on [`GroupedHashAggregateStream`]
struct SkipAggregationProbe {
- /// Number of processed input rows
+ /// Number of processed input rows (updated during probing)
input_rows: usize,
- /// Number of total group values for `input_rows`
+ /// Number of total group values for `input_rows` (updated during probing)
num_groups: usize,
- /// Aggregation ratio check should be performed only when the
- /// number of input rows exceeds this threshold
+ /// Aggregation ratio check performed when the number of input rows exceeds
+ /// this threshold (from `SessionConfig`)
probe_rows_threshold: usize,
- /// Maximum allowed value of `input_rows` / `num_groups` to
- /// continue aggregation
+ /// Maximum ratio of `input_rows` / `num_groups` to continue aggregation
Review Comment:
Yes, the numerator and denominator in this comment should be swapped (I've double checked `Config`, they are correct). Thank you!
--
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] Improve comments in row_hash.rs for skipping aggregation [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11820:
URL: https://github.com/apache/datafusion/pull/11820#discussion_r1709270042
##########
datafusion/physical-plan/src/aggregates/row_hash.rs:
##########
@@ -100,22 +100,24 @@ struct SpillState {
///
/// See "partial aggregation" discussion on [`GroupedHashAggregateStream`]
struct SkipAggregationProbe {
- /// Number of processed input rows
+ /// Number of processed input rows (updated during probing)
input_rows: usize,
- /// Number of total group values for `input_rows`
+ /// Number of total group values for `input_rows` (updated during probing)
num_groups: usize,
- /// Aggregation ratio check should be performed only when the
- /// number of input rows exceeds this threshold
+ /// Aggregation ratio check performed when the number of input rows exceeds
+ /// this threshold (from `SessionConfig`)
probe_rows_threshold: usize,
- /// Maximum allowed value of `input_rows` / `num_groups` to
- /// continue aggregation
+ /// Maximum ratio of `input_rows` / `num_groups` to continue aggregation
Review Comment:
Yes, I agree -- thank you for the catch. @korowa is that your understanding too?
--
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] Improve comments in row_hash.rs for skipping aggregation [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #11820:
URL: https://github.com/apache/datafusion/pull/11820#discussion_r1706347704
##########
datafusion/physical-plan/src/aggregates/row_hash.rs:
##########
@@ -100,22 +100,24 @@ struct SpillState {
///
/// See "partial aggregation" discussion on [`GroupedHashAggregateStream`]
struct SkipAggregationProbe {
- /// Number of processed input rows
+ /// Number of processed input rows (updated during probing)
input_rows: usize,
- /// Number of total group values for `input_rows`
+ /// Number of total group values for `input_rows` (updated during probing)
num_groups: usize,
- /// Aggregation ratio check should be performed only when the
- /// number of input rows exceeds this threshold
+ /// Aggregation ratio check performed when the number of input rows exceeds
+ /// this threshold (from `SessionConfig`)
probe_rows_threshold: usize,
- /// Maximum allowed value of `input_rows` / `num_groups` to
- /// continue aggregation
+ /// Maximum ratio of `input_rows` / `num_groups` to continue aggregation
Review Comment:
The ratio seems to be stated inversely here?
```suggestion
/// Maximum ratio of `num_groups` to `input_rows` for continuing aggregation
```
My understanding is based on the following code:
```rust
self.should_skip = self.num_groups as f64 / self.input_rows as f64
>= self.probe_ratio_threshold;
```
--
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] Minor: Improve comments in row_hash.rs for skipping aggregation [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11820:
URL: https://github.com/apache/datafusion/pull/11820#issuecomment-2284066506
Thanks all
--
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] Improve comments in row_hash.rs for skipping aggregation [datafusion]
Posted by "korowa (via GitHub)" <gi...@apache.org>.
korowa commented on code in PR #11820:
URL: https://github.com/apache/datafusion/pull/11820#discussion_r1711823815
##########
datafusion/physical-plan/src/aggregates/row_hash.rs:
##########
@@ -100,22 +100,24 @@ struct SpillState {
///
/// See "partial aggregation" discussion on [`GroupedHashAggregateStream`]
struct SkipAggregationProbe {
- /// Number of processed input rows
+ /// Number of processed input rows (updated during probing)
input_rows: usize,
- /// Number of total group values for `input_rows`
+ /// Number of total group values for `input_rows` (updated during probing)
num_groups: usize,
- /// Aggregation ratio check should be performed only when the
- /// number of input rows exceeds this threshold
+ /// Aggregation ratio check performed when the number of input rows exceeds
+ /// this threshold (from `SessionConfig`)
probe_rows_threshold: usize,
- /// Maximum allowed value of `input_rows` / `num_groups` to
- /// continue aggregation
+ /// Maximum ratio of `num_groups` to `input_rows` for continuing aggregation
+ /// (from `SessionConfig`). If the ratio exceeds this value, aggregation
+ /// is skipped and input rows are directly converted to output
probe_ratio_threshold: f64,
- /// Flag indicating that further data aggregation mey be skipped
+ /// Flag indicating further data aggregation mey be skipped (decision made
+ /// when probing complete)
should_skip: bool,
- /// Flag indicating that further updates of `SkipAggregationProbe`
- /// state won't make any effect
+ /// Flag indicating further updates of `SkipAggregationProbe` state won't
+ /// make any effect (decision made when probing complete)
Review Comment:
```suggestion
/// make any effect (set either while probing or on probing completion)
```
##########
datafusion/physical-plan/src/aggregates/row_hash.rs:
##########
@@ -100,22 +100,24 @@ struct SpillState {
///
/// See "partial aggregation" discussion on [`GroupedHashAggregateStream`]
struct SkipAggregationProbe {
- /// Number of processed input rows
+ /// Number of processed input rows (updated during probing)
input_rows: usize,
- /// Number of total group values for `input_rows`
+ /// Number of total group values for `input_rows` (updated during probing)
num_groups: usize,
- /// Aggregation ratio check should be performed only when the
- /// number of input rows exceeds this threshold
+ /// Aggregation ratio check performed when the number of input rows exceeds
+ /// this threshold (from `SessionConfig`)
probe_rows_threshold: usize,
- /// Maximum allowed value of `input_rows` / `num_groups` to
- /// continue aggregation
+ /// Maximum ratio of `num_groups` to `input_rows` for continuing aggregation
+ /// (from `SessionConfig`). If the ratio exceeds this value, aggregation
+ /// is skipped and input rows are directly converted to output
probe_ratio_threshold: f64,
- /// Flag indicating that further data aggregation mey be skipped
+ /// Flag indicating further data aggregation mey be skipped (decision made
Review Comment:
```suggestion
/// Flag indicating further data aggregation may be skipped (decision made
```
--
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