You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/06/30 13:22:15 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #6810: Refactor Decimal128 averaging code to be vectorizable (and easier to …
alamb opened a new pull request, #6810:
URL: https://github.com/apache/arrow-datafusion/pull/6810
…read)
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Related to https://github.com/apache/arrow-datafusion/issues/4973
# Rationale for this change
While working on a POC for new grouping code, I found I needed to call the decimal averaging code, but could not do so in a performant (vectorized) way because it created a `ScalarValue`
# What changes are included in this PR?
1. Pull out the decimal average code into its own structure
2. Add comments that explain what is happening
3. Refactor `calculate_result_decimal_for_avg`
# 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
4. 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?
<!--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6810: Refactor Decimal128 averaging code to be vectorizable (and easier to …
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6810:
URL: https://github.com/apache/arrow-datafusion/pull/6810#discussion_r1247860901
##########
datafusion/physical-expr/src/aggregate/utils.rs:
##########
@@ -37,45 +37,107 @@ pub fn get_accum_scalar_values_as_arrays(
.collect::<Vec<_>>())
}
-pub fn calculate_result_decimal_for_avg(
Review Comment:
This function is called once per output row in the current code. I want to be able to perform the setup calculations once and then call the minimal code per row in a loop. Thus this refactor
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6810: Refactor Decimal128 averaging code to be vectorizable (and easier to …
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6810:
URL: https://github.com/apache/arrow-datafusion/pull/6810#discussion_r1247861258
##########
datafusion/physical-expr/src/aggregate/utils.rs:
##########
@@ -37,45 +37,107 @@ pub fn get_accum_scalar_values_as_arrays(
.collect::<Vec<_>>())
}
-pub fn calculate_result_decimal_for_avg(
- lit_value: i128,
- count: i128,
- scale: i8,
- target_type: &DataType,
-) -> Result<ScalarValue> {
- match target_type {
- DataType::Decimal128(p, s) => {
- // Different precision for decimal128 can store different range of value.
- // For example, the precision is 3, the max of value is `999` and the min
- // value is `-999`
- let (target_mul, target_min, target_max) = (
- 10_i128.pow(*s as u32),
- MIN_DECIMAL_FOR_EACH_PRECISION[*p as usize - 1],
- MAX_DECIMAL_FOR_EACH_PRECISION[*p as usize - 1],
- );
- let lit_scale_mul = 10_i128.pow(scale as u32);
- if target_mul >= lit_scale_mul {
- if let Some(value) = lit_value.checked_mul(target_mul / lit_scale_mul) {
- let new_value = value / count;
- if new_value >= target_min && new_value <= target_max {
- Ok(ScalarValue::Decimal128(Some(new_value), *p, *s))
- } else {
- Err(DataFusionError::Execution(
- "Arithmetic Overflow in AvgAccumulator".to_string(),
- ))
- }
- } else {
- // can't convert the lit decimal to the returned data type
- Err(DataFusionError::Execution(
- "Arithmetic Overflow in AvgAccumulator".to_string(),
- ))
- }
+/// Computes averages for `Decimal128` values, checking for overflow
+///
+/// This is needed because different precisions for Decimal128 can
+/// store different ranges of values and thus sum/count may not fit in
+/// the target type.
+///
+/// For example, the precision is 3, the max of value is `999` and the min
+/// value is `-999`
+pub(crate) struct Decimal128Averager {
+ /// scale factor for sum values (10^sum_scale)
+ sum_mul: i128,
+ /// scale factor for target (10^target_scale)
+ target_mul: i128,
+ /// The minimum output value possible to represent with the target precision
+ target_min: i128,
+ /// The maximum output value possible to represent with the target precision
+ target_max: i128,
+}
+
+impl Decimal128Averager {
+ /// Create a new `Decimal128Averager`:
+ ///
+ /// * sum_scale: the scale of `sum` values passed to [`Self::avg`]
+ /// * target_precision: the output precision
+ /// * target_precision: the output scale
+ ///
+ /// Errors if the resulting data can not be stored
+ pub fn try_new(
+ sum_scale: i8,
+ target_precision: u8,
+ target_scale: i8,
+ ) -> Result<Self> {
+ let sum_mul = 10_i128.pow(sum_scale as u32);
+ let target_mul = 10_i128.pow(target_scale as u32);
+ let target_min = MIN_DECIMAL_FOR_EACH_PRECISION[target_precision as usize - 1];
+ let target_max = MAX_DECIMAL_FOR_EACH_PRECISION[target_precision as usize - 1];
+
+ if target_mul >= sum_mul {
+ Ok(Self {
+ sum_mul,
+ target_mul,
+ target_min,
+ target_max,
+ })
+ } else {
+ // can't convert the lit decimal to the returned data type
+ Err(DataFusionError::Execution(
+ "Arithmetic Overflow in AvgAccumulator".to_string(),
+ ))
+ }
+ }
+
+ /// Returns the `sum`/`count` as a i128 Decimal128 with
+ /// target_scale and target_precision and reporting overflow.
+ ///
+ /// * sum: The total sum value stored as Decimal128 with sum_scale
+ /// (passed to `Self::try_new`)
+ /// * count: total count, stored as a i128 (*NOT* a Decimal128 value)
+ #[inline(always)]
+ pub fn avg(&self, sum: i128, count: i128) -> Result<i128> {
Review Comment:
My goal is to call this function in a loop
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #6810: Refactor Decimal128 averaging code to be vectorizable (and easier to read)
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6810:
URL: https://github.com/apache/arrow-datafusion/pull/6810#issuecomment-1616602087
> Refactoring looks good to me. I think the purpose is to call refactored function (avg) in other code (not this change) in vectorizable way?
That is right -- in case you are interested, the location is here (and it shows very promising results)
https://github.com/alamb/arrow-datafusion/blob/35b413239a1fdad1663231cddc73501748404835/datafusion/physical-expr/src/aggregate/average.rs#L172-L181
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a diff in pull request #6810: Refactor Decimal128 averaging code to be vectorizable (and easier to read)
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6810:
URL: https://github.com/apache/arrow-datafusion/pull/6810#discussion_r1248986487
##########
datafusion/physical-expr/src/aggregate/utils.rs:
##########
@@ -37,45 +37,107 @@ pub fn get_accum_scalar_values_as_arrays(
.collect::<Vec<_>>())
}
-pub fn calculate_result_decimal_for_avg(
- lit_value: i128,
- count: i128,
- scale: i8,
- target_type: &DataType,
-) -> Result<ScalarValue> {
- match target_type {
- DataType::Decimal128(p, s) => {
- // Different precision for decimal128 can store different range of value.
- // For example, the precision is 3, the max of value is `999` and the min
- // value is `-999`
- let (target_mul, target_min, target_max) = (
- 10_i128.pow(*s as u32),
- MIN_DECIMAL_FOR_EACH_PRECISION[*p as usize - 1],
- MAX_DECIMAL_FOR_EACH_PRECISION[*p as usize - 1],
- );
- let lit_scale_mul = 10_i128.pow(scale as u32);
- if target_mul >= lit_scale_mul {
- if let Some(value) = lit_value.checked_mul(target_mul / lit_scale_mul) {
- let new_value = value / count;
- if new_value >= target_min && new_value <= target_max {
- Ok(ScalarValue::Decimal128(Some(new_value), *p, *s))
- } else {
- Err(DataFusionError::Execution(
- "Arithmetic Overflow in AvgAccumulator".to_string(),
- ))
- }
- } else {
- // can't convert the lit decimal to the returned data type
- Err(DataFusionError::Execution(
- "Arithmetic Overflow in AvgAccumulator".to_string(),
- ))
- }
+/// Computes averages for `Decimal128` values, checking for overflow
+///
+/// This is needed because different precisions for Decimal128 can
+/// store different ranges of values and thus sum/count may not fit in
+/// the target type.
+///
+/// For example, the precision is 3, the max of value is `999` and the min
+/// value is `-999`
+pub(crate) struct Decimal128Averager {
+ /// scale factor for sum values (10^sum_scale)
+ sum_mul: i128,
+ /// scale factor for target (10^target_scale)
+ target_mul: i128,
+ /// The minimum output value possible to represent with the target precision
+ target_min: i128,
+ /// The maximum output value possible to represent with the target precision
+ target_max: i128,
+}
+
+impl Decimal128Averager {
+ /// Create a new `Decimal128Averager`:
+ ///
+ /// * sum_scale: the scale of `sum` values passed to [`Self::avg`]
+ /// * target_precision: the output precision
+ /// * target_precision: the output scale
+ ///
+ /// Errors if the resulting data can not be stored
+ pub fn try_new(
+ sum_scale: i8,
+ target_precision: u8,
+ target_scale: i8,
+ ) -> Result<Self> {
+ let sum_mul = 10_i128.pow(sum_scale as u32);
+ let target_mul = 10_i128.pow(target_scale as u32);
+ let target_min = MIN_DECIMAL_FOR_EACH_PRECISION[target_precision as usize - 1];
+ let target_max = MAX_DECIMAL_FOR_EACH_PRECISION[target_precision as usize - 1];
+
+ if target_mul >= sum_mul {
+ Ok(Self {
+ sum_mul,
+ target_mul,
+ target_min,
+ target_max,
+ })
+ } else {
+ // can't convert the lit decimal to the returned data type
+ Err(DataFusionError::Execution(
+ "Arithmetic Overflow in AvgAccumulator".to_string(),
+ ))
+ }
+ }
+
+ /// Returns the `sum`/`count` as a i128 Decimal128 with
+ /// target_scale and target_precision and reporting overflow.
+ ///
+ /// * sum: The total sum value stored as Decimal128 with sum_scale
+ /// (passed to `Self::try_new`)
+ /// * count: total count, stored as a i128 (*NOT* a Decimal128 value)
+ #[inline(always)]
+ pub fn avg(&self, sum: i128, count: i128) -> Result<i128> {
Review Comment:
I guess that you mean you will call this in a loop in new grouping 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a diff in pull request #6810: Refactor Decimal128 averaging code to be vectorizable (and easier to read)
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6810:
URL: https://github.com/apache/arrow-datafusion/pull/6810#discussion_r1248985787
##########
datafusion/physical-expr/src/aggregate/utils.rs:
##########
@@ -37,45 +37,107 @@ pub fn get_accum_scalar_values_as_arrays(
.collect::<Vec<_>>())
}
-pub fn calculate_result_decimal_for_avg(
Review Comment:
Is it called per output row? Isn't it called in `evaluate`? I think `evaluate` is called only to get final aggregate value?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #6810: Refactor Decimal128 averaging code to be vectorizable (and easier to read)
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6810:
URL: https://github.com/apache/arrow-datafusion/pull/6810#issuecomment-1614952110
> I'll review this soon or later in weekend.
Thanks @viirya -- no rust from my perspective
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6810: Refactor Decimal128 averaging code to be vectorizable (and easier to read)
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6810:
URL: https://github.com/apache/arrow-datafusion/pull/6810#discussion_r1249441606
##########
datafusion/physical-expr/src/aggregate/utils.rs:
##########
@@ -37,45 +37,107 @@ pub fn get_accum_scalar_values_as_arrays(
.collect::<Vec<_>>())
}
-pub fn calculate_result_decimal_for_avg(
- lit_value: i128,
- count: i128,
- scale: i8,
- target_type: &DataType,
-) -> Result<ScalarValue> {
- match target_type {
- DataType::Decimal128(p, s) => {
- // Different precision for decimal128 can store different range of value.
- // For example, the precision is 3, the max of value is `999` and the min
- // value is `-999`
- let (target_mul, target_min, target_max) = (
- 10_i128.pow(*s as u32),
- MIN_DECIMAL_FOR_EACH_PRECISION[*p as usize - 1],
- MAX_DECIMAL_FOR_EACH_PRECISION[*p as usize - 1],
- );
- let lit_scale_mul = 10_i128.pow(scale as u32);
- if target_mul >= lit_scale_mul {
- if let Some(value) = lit_value.checked_mul(target_mul / lit_scale_mul) {
- let new_value = value / count;
- if new_value >= target_min && new_value <= target_max {
- Ok(ScalarValue::Decimal128(Some(new_value), *p, *s))
- } else {
- Err(DataFusionError::Execution(
- "Arithmetic Overflow in AvgAccumulator".to_string(),
- ))
- }
- } else {
- // can't convert the lit decimal to the returned data type
- Err(DataFusionError::Execution(
- "Arithmetic Overflow in AvgAccumulator".to_string(),
- ))
- }
+/// Computes averages for `Decimal128` values, checking for overflow
+///
+/// This is needed because different precisions for Decimal128 can
+/// store different ranges of values and thus sum/count may not fit in
+/// the target type.
+///
+/// For example, the precision is 3, the max of value is `999` and the min
+/// value is `-999`
+pub(crate) struct Decimal128Averager {
+ /// scale factor for sum values (10^sum_scale)
+ sum_mul: i128,
+ /// scale factor for target (10^target_scale)
+ target_mul: i128,
+ /// The minimum output value possible to represent with the target precision
+ target_min: i128,
+ /// The maximum output value possible to represent with the target precision
+ target_max: i128,
+}
+
+impl Decimal128Averager {
+ /// Create a new `Decimal128Averager`:
+ ///
+ /// * sum_scale: the scale of `sum` values passed to [`Self::avg`]
+ /// * target_precision: the output precision
+ /// * target_precision: the output scale
+ ///
+ /// Errors if the resulting data can not be stored
+ pub fn try_new(
+ sum_scale: i8,
+ target_precision: u8,
+ target_scale: i8,
+ ) -> Result<Self> {
+ let sum_mul = 10_i128.pow(sum_scale as u32);
+ let target_mul = 10_i128.pow(target_scale as u32);
+ let target_min = MIN_DECIMAL_FOR_EACH_PRECISION[target_precision as usize - 1];
+ let target_max = MAX_DECIMAL_FOR_EACH_PRECISION[target_precision as usize - 1];
+
+ if target_mul >= sum_mul {
+ Ok(Self {
+ sum_mul,
+ target_mul,
+ target_min,
+ target_max,
+ })
+ } else {
+ // can't convert the lit decimal to the returned data type
+ Err(DataFusionError::Execution(
+ "Arithmetic Overflow in AvgAccumulator".to_string(),
+ ))
+ }
+ }
+
+ /// Returns the `sum`/`count` as a i128 Decimal128 with
+ /// target_scale and target_precision and reporting overflow.
+ ///
+ /// * sum: The total sum value stored as Decimal128 with sum_scale
+ /// (passed to `Self::try_new`)
+ /// * count: total count, stored as a i128 (*NOT* a Decimal128 value)
+ #[inline(always)]
+ pub fn avg(&self, sum: i128, count: i128) -> Result<i128> {
Review Comment:
Exactly -- https://github.com/apache/arrow-datafusion/pull/6810#issuecomment-1616602087
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6810: Refactor Decimal128 averaging code to be vectorizable (and easier to read)
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6810:
URL: https://github.com/apache/arrow-datafusion/pull/6810#discussion_r1249441214
##########
datafusion/physical-expr/src/aggregate/utils.rs:
##########
@@ -37,45 +37,107 @@ pub fn get_accum_scalar_values_as_arrays(
.collect::<Vec<_>>())
}
-pub fn calculate_result_decimal_for_avg(
Review Comment:
> Isn't it called in evaluate?
Yes
> I think evaluate is called only to get final aggregate value?
Correct -- it is called *for each group* to get the final aggregate value. The ASCII art on this comment https://github.com/apache/arrow-datafusion/issues/4973#issuecomment-1608068287 might help to understand why
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a diff in pull request #6810: Refactor Decimal128 averaging code to be vectorizable (and easier to read)
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6810:
URL: https://github.com/apache/arrow-datafusion/pull/6810#discussion_r1248983317
##########
datafusion/physical-expr/src/aggregate/utils.rs:
##########
@@ -37,45 +37,107 @@ pub fn get_accum_scalar_values_as_arrays(
.collect::<Vec<_>>())
}
-pub fn calculate_result_decimal_for_avg(
- lit_value: i128,
- count: i128,
- scale: i8,
- target_type: &DataType,
-) -> Result<ScalarValue> {
- match target_type {
- DataType::Decimal128(p, s) => {
- // Different precision for decimal128 can store different range of value.
- // For example, the precision is 3, the max of value is `999` and the min
- // value is `-999`
- let (target_mul, target_min, target_max) = (
- 10_i128.pow(*s as u32),
- MIN_DECIMAL_FOR_EACH_PRECISION[*p as usize - 1],
- MAX_DECIMAL_FOR_EACH_PRECISION[*p as usize - 1],
- );
- let lit_scale_mul = 10_i128.pow(scale as u32);
- if target_mul >= lit_scale_mul {
- if let Some(value) = lit_value.checked_mul(target_mul / lit_scale_mul) {
- let new_value = value / count;
- if new_value >= target_min && new_value <= target_max {
- Ok(ScalarValue::Decimal128(Some(new_value), *p, *s))
- } else {
- Err(DataFusionError::Execution(
- "Arithmetic Overflow in AvgAccumulator".to_string(),
- ))
- }
- } else {
- // can't convert the lit decimal to the returned data type
- Err(DataFusionError::Execution(
- "Arithmetic Overflow in AvgAccumulator".to_string(),
- ))
- }
+/// Computes averages for `Decimal128` values, checking for overflow
+///
+/// This is needed because different precisions for Decimal128 can
+/// store different ranges of values and thus sum/count may not fit in
+/// the target type.
+///
+/// For example, the precision is 3, the max of value is `999` and the min
+/// value is `-999`
+pub(crate) struct Decimal128Averager {
+ /// scale factor for sum values (10^sum_scale)
+ sum_mul: i128,
+ /// scale factor for target (10^target_scale)
+ target_mul: i128,
+ /// The minimum output value possible to represent with the target precision
+ target_min: i128,
+ /// The maximum output value possible to represent with the target precision
+ target_max: i128,
+}
+
+impl Decimal128Averager {
+ /// Create a new `Decimal128Averager`:
+ ///
+ /// * sum_scale: the scale of `sum` values passed to [`Self::avg`]
+ /// * target_precision: the output precision
+ /// * target_precision: the output scale
Review Comment:
```suggestion
/// * target_scale: the output scale
```
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #6810: Refactor Decimal128 averaging code to be vectorizable (and easier to read)
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6810:
URL: https://github.com/apache/arrow-datafusion/pull/6810
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org