Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
alamb commented on PR #17255: URL: https://github.com/apache/datafusion/pull/17255#issuecomment-3218005743 🚀 -- 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]
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
Jefffrey merged PR #17255: URL: https://github.com/apache/datafusion/pull/17255 -- 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]
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
Jefffrey commented on PR #17255: URL: https://github.com/apache/datafusion/pull/17255#issuecomment-3216761513 > I updated the pin and merged up from main Cheers 👍 -- 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]
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
alamb commented on PR #17255: URL: https://github.com/apache/datafusion/pull/17255#issuecomment-3216751873 I updated the pin and merged up from main -- 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]
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
alamb commented on PR #17255: URL: https://github.com/apache/datafusion/pull/17255#issuecomment-3216745731 ```\Finished `release-nonlto` profile [optimized] target(s) in 2m 09s Running bin/sqllogictests.rs (target/release-nonlto/deps/sqllogictests-78c77c9b80ab3916) Completed 942 test files in 4 minutes andrewlamb@Andrews-MacBook-Pro-3:~/Software/datafusion$ ``` 👍 -- 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]
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
alamb commented on code in PR #17255:
URL: https://github.com/apache/datafusion/pull/17255#discussion_r2295810676
##
datafusion/sqllogictest/test_files/aggregate.slt:
##
@@ -7387,3 +7389,59 @@ FROM (VALUES ('a'), ('d'), ('c'), ('a')) t(a_varchar);
query error Error during planning: ORDER BY and WITHIN GROUP clauses cannot be
used together in the same aggregate function
SELECT array_agg(a_varchar order by a_varchar) WITHIN GROUP (ORDER BY
a_varchar)
FROM (VALUES ('a'), ('d'), ('c'), ('a')) t(a_varchar);
+
+# distinct average
Review Comment:
❤️
--
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]
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
alamb commented on PR #17255: URL: https://github.com/apache/datafusion/pull/17255#issuecomment-3216735334 I think all we need now is to merge https://github.com/apache/datafusion-testing/pull/11 and then update the `datafusion-testing` pin on this PR. I am testing locally with the changes from https://github.com/apache/datafusion-testing/pull/11 using: ```shell INCLUDE_SQLITE=true cargo test --profile release-nonlto --test sqllogictests ``` -- 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]
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
Jefffrey commented on code in PR #17255:
URL: https://github.com/apache/datafusion/pull/17255#discussion_r2295610306
##
datafusion/sqllogictest/test_files/aggregate.slt:
##
@@ -7387,3 +7389,58 @@ FROM (VALUES ('a'), ('d'), ('c'), ('a')) t(a_varchar);
query error Error during planning: ORDER BY and WITHIN GROUP clauses cannot be
used together in the same aggregate function
SELECT array_agg(a_varchar order by a_varchar) WITHIN GROUP (ORDER BY
a_varchar)
FROM (VALUES ('a'), ('d'), ('c'), ('a')) t(a_varchar);
+
+# distinct average
+statement ok
+create table distinct_avg (a int, b int) as values
+ (null, null),
+ (1, 1),
+ (2, 2),
+ (3, 3),
+ (4, 4),
Review Comment:
Addressed those points in this commit:
https://github.com/apache/datafusion/pull/17255/commits/3abb4b729b87c80a6c6083caf2c68f221d97b96c
--
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]
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
Jefffrey commented on PR #17255: URL: https://github.com/apache/datafusion/pull/17255#issuecomment-3216228535 > This will I assume require regenerating the extended slt files in datafusion-testing? Yep, refer to https://github.com/apache/datafusion-testing/pull/11 -- 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]
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
Jefffrey commented on code in PR #17255:
URL: https://github.com/apache/datafusion/pull/17255#discussion_r2295094117
##
datafusion/sqllogictest/test_files/aggregate.slt:
##
@@ -7387,3 +7389,58 @@ FROM (VALUES ('a'), ('d'), ('c'), ('a')) t(a_varchar);
query error Error during planning: ORDER BY and WITHIN GROUP clauses cannot be
used together in the same aggregate function
SELECT array_agg(a_varchar order by a_varchar) WITHIN GROUP (ORDER BY
a_varchar)
FROM (VALUES ('a'), ('d'), ('c'), ('a')) t(a_varchar);
+
+# distinct average
+statement ok
+create table distinct_avg (a int, b int) as values
+ (null, null),
+ (1, 1),
+ (2, 2),
+ (3, 3),
+ (4, 4),
Review Comment:
Will work on adding these cases 👍
--
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]
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
Jefffrey commented on code in PR #17255:
URL: https://github.com/apache/datafusion/pull/17255#discussion_r2295094013
##
datafusion/functions-aggregate/src/average.rs:
##
@@ -114,72 +115,88 @@ impl AggregateUDFImpl for Avg {
}
fn accumulator(&self, acc_args: AccumulatorArgs) -> Result> {
-if acc_args.is_distinct {
-return exec_err!("avg(DISTINCT) aggregations are not available");
-}
+let data_type = acc_args.exprs[0].data_type(acc_args.schema)?;
use DataType::*;
-let data_type = acc_args.exprs[0].data_type(acc_args.schema)?;
// instantiate specialized accumulator based for the type
-match (&data_type, acc_args.return_field.data_type()) {
-(Float64, Float64) => Ok(Boxdefault()),
-(
-Decimal128(sum_precision, sum_scale),
-Decimal128(target_precision, target_scale),
-) => Ok(Box::new(DecimalAvgAccumulator:: {
-sum: None,
-count: 0,
-sum_scale: *sum_scale,
-sum_precision: *sum_precision,
-target_precision: *target_precision,
-target_scale: *target_scale,
-})),
-
-(
-Decimal256(sum_precision, sum_scale),
-Decimal256(target_precision, target_scale),
-) => Ok(Box::new(DecimalAvgAccumulator:: {
-sum: None,
-count: 0,
-sum_scale: *sum_scale,
-sum_precision: *sum_precision,
-target_precision: *target_precision,
-target_scale: *target_scale,
-})),
-
-(Duration(time_unit), Duration(result_unit)) => {
-Ok(Box::new(DurationAvgAccumulator {
+if acc_args.is_distinct {
+match &data_type {
+// Numeric types are converted to Float64 via
`coerce_avg_type` during logical plan creation
+Float64 =>
Ok(Box::new(Float64DistinctAvgAccumulator::default())),
+_ => exec_err!("AVG(DISTINCT) for {} not supported",
data_type),
+}
+} else {
+match (&data_type, acc_args.return_field.data_type()) {
+(Float64, Float64) => Ok(Boxdefault()),
+(
+Decimal128(sum_precision, sum_scale),
+Decimal128(target_precision, target_scale),
+) => Ok(Box::new(DecimalAvgAccumulator:: {
sum: None,
count: 0,
-time_unit: *time_unit,
-result_unit: *result_unit,
-}))
-}
+sum_scale: *sum_scale,
+sum_precision: *sum_precision,
+target_precision: *target_precision,
+target_scale: *target_scale,
+})),
+
+(
+Decimal256(sum_precision, sum_scale),
+Decimal256(target_precision, target_scale),
+) => Ok(Box::new(DecimalAvgAccumulator:: {
+sum: None,
+count: 0,
+sum_scale: *sum_scale,
+sum_precision: *sum_precision,
+target_precision: *target_precision,
+target_scale: *target_scale,
+})),
+
+(Duration(time_unit), Duration(result_unit)) => {
+Ok(Box::new(DurationAvgAccumulator {
+sum: None,
+count: 0,
+time_unit: *time_unit,
+result_unit: *result_unit,
+}))
+}
-_ => exec_err!(
-"AvgAccumulator for ({} --> {})",
-&data_type,
-acc_args.return_field.data_type()
-),
+_ => exec_err!(
+"AvgAccumulator for ({} --> {})",
+&data_type,
+acc_args.return_field.data_type()
+),
+}
}
}
fn state_fields(&self, args: StateFieldsArgs) -> Result> {
-Ok(vec![
-Field::new(
-format_state_name(args.name, "count"),
-DataType::UInt64,
-true,
-),
-Field::new(
-format_state_name(args.name, "sum"),
-args.input_fields[0].data_type().clone(),
-true,
-),
-]
-.into_iter()
-.map(Arc::new)
-.collect())
+if args.is_distinct {
+// Copied from
datafusion_functions_aggregate::sum::Sum::state_fields
+// since the accumulator uses DistinctSumAccumulator internally.
+Ok(vec![Field::new_list(
+format_state_name(args.name, "sum distinct"),
+
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
alamb commented on code in PR #17255:
URL: https://github.com/apache/datafusion/pull/17255#discussion_r2294598360
##
datafusion/sqllogictest/test_files/aggregate.slt:
##
@@ -7387,3 +7389,58 @@ FROM (VALUES ('a'), ('d'), ('c'), ('a')) t(a_varchar);
query error Error during planning: ORDER BY and WITHIN GROUP clauses cannot be
used together in the same aggregate function
SELECT array_agg(a_varchar order by a_varchar) WITHIN GROUP (ORDER BY
a_varchar)
FROM (VALUES ('a'), ('d'), ('c'), ('a')) t(a_varchar);
+
+# distinct average
+statement ok
+create table distinct_avg (a int, b int) as values
+ (null, null),
+ (1, 1),
+ (2, 2),
+ (3, 3),
+ (4, 4),
Review Comment:
Could you update this test so:
1. The input isn't in order
2. Add a test for floating point values
3. Test for an input that includes at least one null value
4. the values in `b` are different than the values in `b`
##
datafusion/functions-aggregate/src/average.rs:
##
@@ -114,79 +115,95 @@ impl AggregateUDFImpl for Avg {
}
fn accumulator(&self, acc_args: AccumulatorArgs) -> Result> {
-if acc_args.is_distinct {
-return exec_err!("avg(DISTINCT) aggregations are not available");
-}
+let data_type = acc_args.exprs[0].data_type(acc_args.schema)?;
use DataType::*;
-let data_type = acc_args.exprs[0].data_type(acc_args.schema)?;
// instantiate specialized accumulator based for the type
-match (&data_type, acc_args.return_field.data_type()) {
-(Float64, Float64) => Ok(Boxdefault()),
-(
-Decimal128(sum_precision, sum_scale),
-Decimal128(target_precision, target_scale),
-) => Ok(Box::new(DecimalAvgAccumulator:: {
-sum: None,
-count: 0,
-sum_scale: *sum_scale,
-sum_precision: *sum_precision,
-target_precision: *target_precision,
-target_scale: *target_scale,
-})),
-
-(
-Decimal256(sum_precision, sum_scale),
-Decimal256(target_precision, target_scale),
-) => Ok(Box::new(DecimalAvgAccumulator:: {
-sum: None,
-count: 0,
-sum_scale: *sum_scale,
-sum_precision: *sum_precision,
-target_precision: *target_precision,
-target_scale: *target_scale,
-})),
-
-(Duration(time_unit), Duration(result_unit)) => {
-Ok(Box::new(DurationAvgAccumulator {
+if acc_args.is_distinct {
+match &data_type {
+// Numeric types are converted to Float64 via
`coerce_avg_type` during logical plan creation
+Float64 =>
Ok(Box::new(Float64DistinctAvgAccumulator::default())),
+_ => exec_err!("AVG(DISTINCT) for {} not supported",
data_type),
+}
+} else {
+match (&data_type, acc_args.return_field.data_type()) {
+(Float64, Float64) => Ok(Boxdefault()),
+(
+Decimal128(sum_precision, sum_scale),
+Decimal128(target_precision, target_scale),
+) => Ok(Box::new(DecimalAvgAccumulator:: {
sum: None,
count: 0,
-time_unit: *time_unit,
-result_unit: *result_unit,
-}))
-}
+sum_scale: *sum_scale,
+sum_precision: *sum_precision,
+target_precision: *target_precision,
+target_scale: *target_scale,
+})),
+
+(
+Decimal256(sum_precision, sum_scale),
+Decimal256(target_precision, target_scale),
+) => Ok(Box::new(DecimalAvgAccumulator:: {
+sum: None,
+count: 0,
+sum_scale: *sum_scale,
+sum_precision: *sum_precision,
+target_precision: *target_precision,
+target_scale: *target_scale,
+})),
+
+(Duration(time_unit), Duration(result_unit)) => {
+Ok(Box::new(DurationAvgAccumulator {
+sum: None,
+count: 0,
+time_unit: *time_unit,
+result_unit: *result_unit,
+}))
+}
-_ => exec_err!(
-"AvgAccumulator for ({} --> {})",
-&data_type,
-acc_args.return_field.data_type()
-),
+_ => exec_err!(
+"AvgAccumulator for ({} --> {})",
+&data_type,
+acc_args.return_field.data_type()
+),
+}
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
Omega359 commented on PR #17255: URL: https://github.com/apache/datafusion/pull/17255#issuecomment-3215422900 This will I assume require regenerating the extended slt files in datafusion-testing? -- 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]
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
Jefffrey commented on PR #17255: URL: https://github.com/apache/datafusion/pull/17255#issuecomment-3205232658 > Looks like still getting same error for some of the extended tests: > > ``` > # Datafusion - Datafusion expected results: > query error DataFusion error: Arrow error: Invalid argument error: number of columns\(5\) must match number of fields\(4\) in schema > SELECT ALL - AVG ( ALL + col0 ) AS col1 FROM tab0 GROUP BY col0 HAVING + AVG ( DISTINCT - col2 ) IS NULL > ``` > > Will look into this > > Edit: seems to be related to group accumulator support Fixed by https://github.com/apache/datafusion/pull/17255/commits/bc121fbf46639bc6dde7791658cb2f624fc3ea8c -- 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]
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
Jefffrey commented on code in PR #17255:
URL: https://github.com/apache/datafusion/pull/17255#discussion_r2287475500
##
datafusion/functions-aggregate/src/average.rs:
##
@@ -114,79 +115,95 @@ impl AggregateUDFImpl for Avg {
}
fn accumulator(&self, acc_args: AccumulatorArgs) -> Result> {
-if acc_args.is_distinct {
-return exec_err!("avg(DISTINCT) aggregations are not available");
-}
+let data_type = acc_args.exprs[0].data_type(acc_args.schema)?;
use DataType::*;
-let data_type = acc_args.exprs[0].data_type(acc_args.schema)?;
// instantiate specialized accumulator based for the type
-match (&data_type, acc_args.return_field.data_type()) {
-(Float64, Float64) => Ok(Boxdefault()),
-(
-Decimal128(sum_precision, sum_scale),
-Decimal128(target_precision, target_scale),
-) => Ok(Box::new(DecimalAvgAccumulator:: {
-sum: None,
-count: 0,
-sum_scale: *sum_scale,
-sum_precision: *sum_precision,
-target_precision: *target_precision,
-target_scale: *target_scale,
-})),
-
-(
-Decimal256(sum_precision, sum_scale),
-Decimal256(target_precision, target_scale),
-) => Ok(Box::new(DecimalAvgAccumulator:: {
-sum: None,
-count: 0,
-sum_scale: *sum_scale,
-sum_precision: *sum_precision,
-target_precision: *target_precision,
-target_scale: *target_scale,
-})),
-
-(Duration(time_unit), Duration(result_unit)) => {
-Ok(Box::new(DurationAvgAccumulator {
+if acc_args.is_distinct {
+match &data_type {
+// Numeric types are converted to Float64 via
`coerce_avg_type` during logical plan creation
+Float64 =>
Ok(Box::new(Float64DistinctAvgAccumulator::default())),
+_ => exec_err!("AVG(DISTINCT) for {} not supported",
data_type),
+}
+} else {
+match (&data_type, acc_args.return_field.data_type()) {
+(Float64, Float64) => Ok(Boxdefault()),
+(
+Decimal128(sum_precision, sum_scale),
+Decimal128(target_precision, target_scale),
+) => Ok(Box::new(DecimalAvgAccumulator:: {
sum: None,
count: 0,
-time_unit: *time_unit,
-result_unit: *result_unit,
-}))
-}
+sum_scale: *sum_scale,
+sum_precision: *sum_precision,
+target_precision: *target_precision,
+target_scale: *target_scale,
+})),
+
+(
+Decimal256(sum_precision, sum_scale),
+Decimal256(target_precision, target_scale),
+) => Ok(Box::new(DecimalAvgAccumulator:: {
+sum: None,
+count: 0,
+sum_scale: *sum_scale,
+sum_precision: *sum_precision,
+target_precision: *target_precision,
+target_scale: *target_scale,
+})),
+
+(Duration(time_unit), Duration(result_unit)) => {
+Ok(Box::new(DurationAvgAccumulator {
+sum: None,
+count: 0,
+time_unit: *time_unit,
+result_unit: *result_unit,
+}))
+}
-_ => exec_err!(
-"AvgAccumulator for ({} --> {})",
-&data_type,
-acc_args.return_field.data_type()
-),
+_ => exec_err!(
+"AvgAccumulator for ({} --> {})",
+&data_type,
+acc_args.return_field.data_type()
+),
+}
}
}
fn state_fields(&self, args: StateFieldsArgs) -> Result> {
-Ok(vec![
-Field::new(
-format_state_name(args.name, "count"),
-DataType::UInt64,
-true,
-),
-Field::new(
-format_state_name(args.name, "sum"),
-args.input_fields[0].data_type().clone(),
-true,
-),
-]
-.into_iter()
-.map(Arc::new)
-.collect())
+if args.is_distinct {
+// Copied from
datafusion_functions_aggregate::sum::Sum::state_fields
+// since the accumulator uses DistinctSumAccumulator internally.
+Ok(vec![Field::new_list(
+format_state_name(args.name, "sum distinct"),
+
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
Jefffrey commented on PR #17255: URL: https://github.com/apache/datafusion/pull/17255#issuecomment-3204604429 Looks like still getting same error for some of the extended tests: ``` # Datafusion - Datafusion expected results: query error DataFusion error: Arrow error: Invalid argument error: number of columns\(5\) must match number of fields\(4\) in schema SELECT ALL - AVG ( ALL + col0 ) AS col1 FROM tab0 GROUP BY col0 HAVING + AVG ( DISTINCT - col2 ) IS NULL ``` Will look into this -- 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]
Re: [PR] Support `avg(distinct)` for `float64` type [datafusion]
Jefffrey commented on code in PR #17255:
URL: https://github.com/apache/datafusion/pull/17255#discussion_r2287004655
##
datafusion/functions-aggregate/src/average.rs:
##
@@ -114,72 +115,88 @@ impl AggregateUDFImpl for Avg {
}
fn accumulator(&self, acc_args: AccumulatorArgs) -> Result> {
-if acc_args.is_distinct {
-return exec_err!("avg(DISTINCT) aggregations are not available");
-}
+let data_type = acc_args.exprs[0].data_type(acc_args.schema)?;
use DataType::*;
-let data_type = acc_args.exprs[0].data_type(acc_args.schema)?;
// instantiate specialized accumulator based for the type
-match (&data_type, acc_args.return_field.data_type()) {
-(Float64, Float64) => Ok(Boxdefault()),
-(
-Decimal128(sum_precision, sum_scale),
-Decimal128(target_precision, target_scale),
-) => Ok(Box::new(DecimalAvgAccumulator:: {
-sum: None,
-count: 0,
-sum_scale: *sum_scale,
-sum_precision: *sum_precision,
-target_precision: *target_precision,
-target_scale: *target_scale,
-})),
-
-(
-Decimal256(sum_precision, sum_scale),
-Decimal256(target_precision, target_scale),
-) => Ok(Box::new(DecimalAvgAccumulator:: {
-sum: None,
-count: 0,
-sum_scale: *sum_scale,
-sum_precision: *sum_precision,
-target_precision: *target_precision,
-target_scale: *target_scale,
-})),
-
-(Duration(time_unit), Duration(result_unit)) => {
-Ok(Box::new(DurationAvgAccumulator {
+if acc_args.is_distinct {
+match &data_type {
+// Numeric types are converted to Float64 via
`coerce_avg_type` during logical plan creation
+Float64 =>
Ok(Box::new(Float64DistinctAvgAccumulator::default())),
+_ => exec_err!("AVG(DISTINCT) for {} not supported",
data_type),
+}
+} else {
+match (&data_type, acc_args.return_field.data_type()) {
+(Float64, Float64) => Ok(Boxdefault()),
+(
+Decimal128(sum_precision, sum_scale),
+Decimal128(target_precision, target_scale),
+) => Ok(Box::new(DecimalAvgAccumulator:: {
sum: None,
count: 0,
-time_unit: *time_unit,
-result_unit: *result_unit,
-}))
-}
+sum_scale: *sum_scale,
+sum_precision: *sum_precision,
+target_precision: *target_precision,
+target_scale: *target_scale,
+})),
+
+(
+Decimal256(sum_precision, sum_scale),
+Decimal256(target_precision, target_scale),
+) => Ok(Box::new(DecimalAvgAccumulator:: {
+sum: None,
+count: 0,
+sum_scale: *sum_scale,
+sum_precision: *sum_precision,
+target_precision: *target_precision,
+target_scale: *target_scale,
+})),
+
+(Duration(time_unit), Duration(result_unit)) => {
+Ok(Box::new(DurationAvgAccumulator {
+sum: None,
+count: 0,
+time_unit: *time_unit,
+result_unit: *result_unit,
+}))
+}
-_ => exec_err!(
-"AvgAccumulator for ({} --> {})",
-&data_type,
-acc_args.return_field.data_type()
-),
+_ => exec_err!(
+"AvgAccumulator for ({} --> {})",
+&data_type,
+acc_args.return_field.data_type()
+),
+}
}
}
fn state_fields(&self, args: StateFieldsArgs) -> Result> {
-Ok(vec![
-Field::new(
-format_state_name(args.name, "count"),
-DataType::UInt64,
-true,
-),
-Field::new(
-format_state_name(args.name, "sum"),
-args.input_fields[0].data_type().clone(),
-true,
-),
-]
-.into_iter()
-.map(Arc::new)
-.collect())
+if args.is_distinct {
+// Copied from
datafusion_functions_aggregate::sum::Sum::state_fields
+// since the accumulator uses DistinctSumAccumulator internally.
+Ok(vec![Field::new_list(
+format_state_name(args.name, "sum distinct"),
+
Re: [PR] Support Avg distinct for `float64` type [datafusion]
qazxcdswe123 closed pull request #15413: Support Avg distinct for `float64` type URL: https://github.com/apache/datafusion/pull/15413 -- 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]
Re: [PR] Support Avg distinct for `float64` type [datafusion]
alamb commented on PR #15413: URL: https://github.com/apache/datafusion/pull/15413#issuecomment-2781566886 > sadly I'm working on my undergrad thesis project at this time and do not have time to investigate this either 😢 , might be back around mid april Good luck with your project / thesis! -- 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]
Re: [PR] Support Avg distinct for `float64` type [datafusion]
qazxcdswe123 commented on PR #15413: URL: https://github.com/apache/datafusion/pull/15413#issuecomment-2780650412 > I regenerated the sqlite tests again and I think an issue I'm seeing with them is actually caused or triggered by this PR. > > Here is an example: > > ```sql > query error DataFusion error: Arrow error: Invalid argument error: number of columns\(1\) must match number of fields\(2\) in schema > SELECT 84 * + - 61 * + AVG ( DISTINCT ( 70 ) ) AS col2 FROM tab4 WHERE ( NULL ) BETWEEN NULL AND col3 > ``` > > If this is in fact a bug not caused by this PR a new issue should be filed. I myself do not have the time to diagnosis the cause of this unfortunately. sadly I'm working on my undergrad thesis project at this time and do not have time to investigate this either 😢 , might be back around mid april -- 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]
Re: [PR] Support Avg distinct for `float64` type [datafusion]
alamb commented on PR #15413: URL: https://github.com/apache/datafusion/pull/15413#issuecomment-2762446804 I merged up from main and will use the new github action to run extended tests -- 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]
Re: [PR] Support Avg distinct for `float64` type [datafusion]
Omega359 commented on PR #15413: URL: https://github.com/apache/datafusion/pull/15413#issuecomment-2779088360 I regenerated the sqlite tests again and I think an issue I'm seeing with them is actually caused or triggered by this PR. Here is an example: ```sql query error DataFusion error: Arrow error: Invalid argument error: number of columns\(1\) must match number of fields\(2\) in schema SELECT 84 * + - 61 * + AVG ( DISTINCT ( 70 ) ) AS col2 FROM tab4 WHERE ( NULL ) BETWEEN NULL AND col3 ``` If this is in fact a bug not caused by this PR a new issue should be filed. I myself do not have the time to diagnosis the cause of this unfortunately. -- 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]
Re: [PR] Support Avg distinct for `float64` type [datafusion]
Omega359 commented on PR #15413: URL: https://github.com/apache/datafusion/pull/15413#issuecomment-2762816764 Looks like it failed? https://github.com/apache/datafusion/actions/runs/14139465370/job/39618247236 -- 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]
Re: [PR] Support Avg distinct for `float64` type [datafusion]
Omega359 commented on PR #15413: URL: https://github.com/apache/datafusion/pull/15413#issuecomment-2762810733 > Run extended tests I see it did trigger but I somehow was expecting feedback in the comments -- 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]
Re: [PR] Support Avg distinct for `float64` type [datafusion]
alamb commented on PR #15413: URL: https://github.com/apache/datafusion/pull/15413#issuecomment-2762447424 Run extended tests -- 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]
Re: [PR] Support Avg distinct for `float64` type [datafusion]
Omega359 commented on PR #15413: URL: https://github.com/apache/datafusion/pull/15413#issuecomment-2752424919 the sqlite tests need updating prior to this issue being pushed to main. Confirmed test failures, here are a few examples: ``` External error: query is expected to fail with error: (regex) DataFusion error: Execution error: avg\(DISTINCT\) aggregations are not available but got error: DataFusion error: Arrow error: Invalid argument error: number of columns(2) must match number of fields(3) in schema [SQL] SELECT - COUNT ( * ) * AVG ( DISTINCT + col3 ) AS col3 FROM tab0 WHERE NOT NULL <= + + col3 at ../../datafusion-testing/data/sqlite/index/random/100/slt_good_1.slt:11104 External error: query is expected to fail, but actually succeed: [SQL] SELECT DISTINCT - COUNT ( * ) * AVG ( DISTINCT + 89 ) col4 FROM tab0 AS cor0 WHERE NULL NOT IN ( CAST ( NULL AS INTEGER ) + - 15 ) at ../../datafusion-testing/data/sqlite/index/random/1000/slt_good_8.slt:17760 External error: query is expected to fail, but actually succeed: [SQL] SELECT - AVG ( DISTINCT 27 ) + - MAX ( DISTINCT col1 ) AS col4 FROM tab0 WHERE NOT NULL <> - + 90 at ../../datafusion-testing/data/sqlite/index/random/1000/slt_good_0.slt:6040 External error: query is expected to fail with error: (regex) DataFusion error: Execution error: avg\(DISTINCT\) aggregations are not available but got error: DataFusion error: Arrow error: Invalid argument error: number of columns(1) must match number of fields(2) in schema [SQL] SELECT ALL - AVG ( DISTINCT 48 ) AS col4 FROM tab0 WHERE NOT - col4 BETWEEN ( NULL ) AND CAST ( 2 AS INTEGER ) at ../../datafusion-testing/data/sqlite/index/random/1000/slt_good_7.slt:38250 ``` -- 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]
Re: [PR] Support Avg distinct for `float64` type [datafusion]
jayzhan211 commented on code in PR #15413:
URL: https://github.com/apache/datafusion/pull/15413#discussion_r2011786304
##
datafusion/functions-aggregate-common/src/aggregate/avg_distinct/numeric.rs:
##
@@ -0,0 +1,78 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::fmt::Debug;
+
+use arrow::array::ArrayRef;
+use arrow::datatypes::Float64Type;
+use datafusion_common::ScalarValue;
+use datafusion_expr_common::accumulator::Accumulator;
+
+use crate::aggregate::sum_distinct::DistinctSumAccumulator;
+
+/// Specialized implementation of `AVG DISTINCT` for Float64 values, leveraging
+/// the existing DistinctSumAccumulator implementation.
+#[derive(Debug)]
+pub struct Float64DistinctAvgAccumulator {
+// We use the DistinctSumAccumulator to handle the set of distinct values
+sum_accumulator: DistinctSumAccumulator,
+}
+
+impl Float64DistinctAvgAccumulator {
+pub fn new() -> datafusion_common::Result {
+Ok(Self {
+sum_accumulator: DistinctSumAccumulatortry_new(
+&arrow::datatypes::DataType::Float64,
Review Comment:
nit: DataType::Float64
##
datafusion/functions-aggregate-common/src/aggregate/sum_distinct/mod.rs:
##
@@ -0,0 +1,22 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Sum distinct accumulator implementations
+
+pub mod numeric;
+
+pub use numeric::DistinctSumAccumulator;
Review Comment:
can we remove mod.rs and use the new style similar to count_distinct and
avg_distinct
`datafusion/functions-aggregate-common/src/aggregate/sum_distinct.rs`
--
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]
