You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Garamda (via GitHub)" <gi...@apache.org> on 2024/11/21 03:36:48 UTC
[PR] Support within group syntax for existing aggregate functions [datafusion]
Garamda opened a new pull request, #13511:
URL: https://github.com/apache/datafusion/pull/13511
## 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.
-->
Closes #11732. (cc. #12824)
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
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?
<!--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2699344130
Pinging @Dandandan for commiter review as they filed the ticket this fix is for.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1938219937
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -336,7 +369,28 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
None,
)?;
let order_by = (!order_by.is_empty()).then_some(order_by);
- let args = self.function_args_to_expr(args, schema, planner_context)?;
+ let mut args =
+ self.function_args_to_expr(args, schema, planner_context)?;
+
+ let within_group = self.order_by_to_sort_expr(
+ within_group,
+ schema,
+ planner_context,
+ false,
+ None,
+ )?;
+
+ // add target column expression in within group clause to function arguments
Review Comment:
The `Signature` of `ApproxPercentileCont` and `ApproxPercentileContWithWeight` are not changed. Instead, the expression in `WITHIN GROUP` clause is added into function arguments while converting `sql expr` to `logical expr`.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1938232489
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -1623,27 +1637,45 @@ b NULL NULL 7732.315789473684
# csv_query_approx_percentile_cont_with_weight
query TI
-SELECT c1, approx_percentile_cont(c3, 0.95) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
+SELECT c1, approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c3) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
----
a 73
b 68
c 122
d 124
e 115
+query TI
+SELECT c1, approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c3 DESC) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
+----
+a -101
+b -114
+c -109
+d -98
+e -93
Review Comment:
`ORDER BY DESC` case is added, because the original function was fixed to only ascending order. [Same case](https://github.com/apache/datafusion/pull/13511/files#diff-a60d69cf923e18b0532e7ba3470c4b728e6b00230b3ea242f3fbe95e899c5ffeR1668-R1674) for `approx_percentile_cont_with_weight` is added.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2823540965
I didn't see the notification of this one. I will find time to review it
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1980817133
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -349,15 +365,49 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
} else {
// User defined aggregate functions (UDAF) have precedence in case it has the same name as a scalar built-in function
if let Some(fm) = self.context_provider.get_aggregate_meta(&name) {
- let order_by = self.order_by_to_sort_expr(
- order_by,
- schema,
- planner_context,
- true,
- None,
- )?;
- let order_by = (!order_by.is_empty()).then_some(order_by);
- let args = self.function_args_to_expr(args, schema, planner_context)?;
+ if fm.is_ordered_set_aggregate() && within_group.is_empty() {
+ return plan_err!("WITHIN GROUP clause is required when calling ordered set aggregate function({})", fm.name());
+ }
+
+ if null_treatment.is_some() && !fm.supports_null_handling_clause() {
+ return plan_err!(
+ "[IGNORE | RESPECT] NULLS are not permitted for {}",
+ fm.name()
+ );
+ }
Review Comment:
> Thanks for bearing with me as a reviewed this, it was a good chance for me to look at new parts of DataFusion.
I appreciate your elaborate review again. 👍
This PR has become much simpler, clearer, and better now.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1976295147
##########
datafusion/functions-aggregate/src/approx_percentile_cont.rs:
##########
@@ -51,29 +52,43 @@ create_func!(ApproxPercentileCont, approx_percentile_cont_udaf);
/// Computes the approximate percentile continuous of a set of numbers
pub fn approx_percentile_cont(
- expression: Expr,
+ within_group: Option<Vec<Sort>>,
Review Comment:
> It would probably be good to add a test for this if there isn't already
Do you mean the test case like this https://github.com/apache/datafusion/pull/13511#discussion_r1938232489 ?
If I misunderstood, please let me know.
Update : I also add [test cases for dataframe function](https://github.com/apache/datafusion/pull/13511/commits/db0355a532aa2aae4d45eebe05264a80fb2fa04a)
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1938232183
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -77,36 +77,50 @@ SELECT approx_distinct(c9) count_c9, approx_distinct(cast(c9 as varchar)) count_
# csv_query_approx_percentile_cont_with_weight
statement error DataFusion error: Error during planning: Failed to coerce arguments to satisfy a call to approx_percentile_cont_with_weight function: coercion from \[Utf8, Int8, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont_with_weight(c1, c2, 0.95) FROM aggregate_test_100
+SELECT approx_percentile_cont_with_weight(c2, 0.95) WITHIN GROUP (ORDER BY c1) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Failed to coerce arguments to satisfy a call to approx_percentile_cont_with_weight function: coercion from \[Int16, Utf8, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont_with_weight(c3, c1, 0.95) FROM aggregate_test_100
+SELECT approx_percentile_cont_with_weight(c1, 0.95) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Failed to coerce arguments to satisfy a call to approx_percentile_cont_with_weight function: coercion from \[Int16, Int8, Utf8\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont_with_weight(c3, c2, c1) FROM aggregate_test_100
+SELECT approx_percentile_cont_with_weight(c2, c1) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
# csv_query_approx_percentile_cont_with_histogram_bins
statement error DataFusion error: External error: This feature is not implemented: Tdigest max_size value for 'APPROX_PERCENTILE_CONT' must be UInt > 0 literal \(got data type Int64\)\.
-SELECT c1, approx_percentile_cont(c3, 0.95, -1000) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
+SELECT c1, approx_percentile_cont(0.95, -1000) WITHIN GROUP (ORDER BY c3) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
statement error DataFusion error: Error during planning: Failed to coerce arguments to satisfy a call to approx_percentile_cont function: coercion from \[Int16, Float64, Utf8\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont(c3, 0.95, c1) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, c1) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Failed to coerce arguments to satisfy a call to approx_percentile_cont function: coercion from \[Int16, Float64, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont(c3, 0.95, 111.1) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, 111.1) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Failed to coerce arguments to satisfy a call to approx_percentile_cont function: coercion from \[Float64, Float64, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont(c12, 0.95, 111.1) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, 111.1) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
statement error DataFusion error: This feature is not implemented: Percentile value for 'APPROX_PERCENTILE_CONT' must be a literal
-SELECT approx_percentile_cont(c12, c12) FROM aggregate_test_100
+SELECT approx_percentile_cont(c12) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
statement error DataFusion error: This feature is not implemented: Tdigest max_size value for 'APPROX_PERCENTILE_CONT' must be a literal
-SELECT approx_percentile_cont(c12, 0.95, c5) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, c5) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
+
+statement error DataFusion error: This feature is not implemented: Conflicting ordering requirements in aggregate functions is not supported
+SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5), approx_percentile_cont(0.2) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
+
+statement error DataFusion error: Error during planning: \[IGNORE | RESPECT\] NULLS are not permitted for approx_percentile_cont
+SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5) IGNORE NULLS FROM aggregate_test_100
+
+statement error DataFusion error: Error during planning: \[IGNORE | RESPECT\] NULLS are not permitted for approx_percentile_cont
+SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5) RESPECT NULLS FROM aggregate_test_100
+
+statement error DataFusion error: This feature is not implemented: Multiple column ordering in WITHIN GROUP clause is not supported
+SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5, c12) FROM aggregate_test_100
# Not supported over sliding windows
-query error This feature is not implemented: Aggregate can not be used as a sliding accumulator because `retract_batch` is not implemented
-SELECT approx_percentile_cont(c3, 0.5) OVER (ROWS BETWEEN 4 PRECEDING AND CURRENT ROW)
+query error DataFusion error: Error during planning: OVER and WITHIN GROUP clause are can not be used together. OVER is for window function, whereas WITHIN GROUP is for ordered set aggregate function
+SELECT approx_percentile_cont(0.5)
+WITHIN GROUP (ORDER BY c3)
+OVER (ROWS BETWEEN 4 PRECEDING AND CURRENT ROW)
Review Comment:
To my knowledge, the `OVER` clause is used for window functions, whereas the `WITHIN GROUP` is for ordered set aggregate functions, and they cannot be used together. So I modified error message here, and error handling like [this](https://github.com/apache/datafusion/pull/13511/files#diff-e4760f867708d4ecd16dd4f38161a39d330c8d55f170e059aaba07317a07e42aR227-R231) accordingly.
**_Do these changes comply with the SQL standard?_**
--
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] Support within group syntax for existing aggregate functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2492389897
FYI @Dandandan
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2826255767
@jayzhan211 Alright, I see. 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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1938232183
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -77,36 +77,50 @@ SELECT approx_distinct(c9) count_c9, approx_distinct(cast(c9 as varchar)) count_
# csv_query_approx_percentile_cont_with_weight
statement error DataFusion error: Error during planning: Failed to coerce arguments to satisfy a call to approx_percentile_cont_with_weight function: coercion from \[Utf8, Int8, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont_with_weight(c1, c2, 0.95) FROM aggregate_test_100
+SELECT approx_percentile_cont_with_weight(c2, 0.95) WITHIN GROUP (ORDER BY c1) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Failed to coerce arguments to satisfy a call to approx_percentile_cont_with_weight function: coercion from \[Int16, Utf8, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont_with_weight(c3, c1, 0.95) FROM aggregate_test_100
+SELECT approx_percentile_cont_with_weight(c1, 0.95) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Failed to coerce arguments to satisfy a call to approx_percentile_cont_with_weight function: coercion from \[Int16, Int8, Utf8\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont_with_weight(c3, c2, c1) FROM aggregate_test_100
+SELECT approx_percentile_cont_with_weight(c2, c1) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
# csv_query_approx_percentile_cont_with_histogram_bins
statement error DataFusion error: External error: This feature is not implemented: Tdigest max_size value for 'APPROX_PERCENTILE_CONT' must be UInt > 0 literal \(got data type Int64\)\.
-SELECT c1, approx_percentile_cont(c3, 0.95, -1000) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
+SELECT c1, approx_percentile_cont(0.95, -1000) WITHIN GROUP (ORDER BY c3) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
statement error DataFusion error: Error during planning: Failed to coerce arguments to satisfy a call to approx_percentile_cont function: coercion from \[Int16, Float64, Utf8\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont(c3, 0.95, c1) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, c1) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Failed to coerce arguments to satisfy a call to approx_percentile_cont function: coercion from \[Int16, Float64, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont(c3, 0.95, 111.1) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, 111.1) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Failed to coerce arguments to satisfy a call to approx_percentile_cont function: coercion from \[Float64, Float64, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont(c12, 0.95, 111.1) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, 111.1) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
statement error DataFusion error: This feature is not implemented: Percentile value for 'APPROX_PERCENTILE_CONT' must be a literal
-SELECT approx_percentile_cont(c12, c12) FROM aggregate_test_100
+SELECT approx_percentile_cont(c12) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
statement error DataFusion error: This feature is not implemented: Tdigest max_size value for 'APPROX_PERCENTILE_CONT' must be a literal
-SELECT approx_percentile_cont(c12, 0.95, c5) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, c5) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
+
+statement error DataFusion error: This feature is not implemented: Conflicting ordering requirements in aggregate functions is not supported
+SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5), approx_percentile_cont(0.2) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
+
+statement error DataFusion error: Error during planning: \[IGNORE | RESPECT\] NULLS are not permitted for approx_percentile_cont
+SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5) IGNORE NULLS FROM aggregate_test_100
+
+statement error DataFusion error: Error during planning: \[IGNORE | RESPECT\] NULLS are not permitted for approx_percentile_cont
+SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5) RESPECT NULLS FROM aggregate_test_100
+
+statement error DataFusion error: This feature is not implemented: Multiple column ordering in WITHIN GROUP clause is not supported
+SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5, c12) FROM aggregate_test_100
# Not supported over sliding windows
-query error This feature is not implemented: Aggregate can not be used as a sliding accumulator because `retract_batch` is not implemented
-SELECT approx_percentile_cont(c3, 0.5) OVER (ROWS BETWEEN 4 PRECEDING AND CURRENT ROW)
+query error DataFusion error: Error during planning: OVER and WITHIN GROUP clause are can not be used together. OVER is for window function, whereas WITHIN GROUP is for ordered set aggregate function
+SELECT approx_percentile_cont(0.5)
+WITHIN GROUP (ORDER BY c3)
+OVER (ROWS BETWEEN 4 PRECEDING AND CURRENT ROW)
Review Comment:
In my knowledge, `OVER` clause is for window function, whereas `WITHIN GROUP` is for ordered set aggregate function, and they can not be used together. So I modified error message here, and error handling like [this](https://github.com/apache/datafusion/pull/13511/files#diff-e4760f867708d4ecd16dd4f38161a39d330c8d55f170e059aaba07317a07e42aR227-R231).
**_Do these changes comply with the SQL standard?_**
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1976295147
##########
datafusion/functions-aggregate/src/approx_percentile_cont.rs:
##########
@@ -51,29 +52,43 @@ create_func!(ApproxPercentileCont, approx_percentile_cont_udaf);
/// Computes the approximate percentile continuous of a set of numbers
pub fn approx_percentile_cont(
- expression: Expr,
+ within_group: Option<Vec<Sort>>,
Review Comment:
> It would probably be good to add a test for this if there isn't already
Do you mean the test case like this https://github.com/apache/datafusion/pull/13511#discussion_r1938232489 ?
If I misunderstood, please let me know.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1980465883
##########
datafusion/expr/src/udaf.rs:
##########
@@ -845,6 +861,19 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
ScalarValue::try_from(data_type)
}
+ /// If this function supports `[IGNORE NULLS | RESPECT NULLS]` clause, return true
+ /// If the function does not, return false
+ /// Otherwise return None (the default)
+ fn supports_null_handling_clause(&self) -> Option<bool> {
+ None
+ }
Review Comment:
Filed https://github.com/apache/datafusion/issues/15006
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1938226776
##########
datafusion/functions-aggregate/src/approx_percentile_cont.rs:
##########
@@ -131,6 +147,19 @@ impl ApproxPercentileCont {
args: AccumulatorArgs,
) -> Result<ApproxPercentileAccumulator> {
let percentile = validate_input_percentile_expr(&args.exprs[1])?;
+
+ let is_descending = args
+ .ordering_req
+ .first()
+ .map(|sort_expr| sort_expr.options.descending)
+ .unwrap_or(false);
+
+ let percentile = if is_descending {
Review Comment:
I used floating point subtraction instead of actual sorting in reverse order, for conciseness.
**_If any slight floating point difference is not permitted (even if this branch passed the tests), please let me know._**
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1938228223
##########
datafusion/functions-aggregate/src/approx_percentile_cont.rs:
##########
@@ -131,6 +147,19 @@ impl ApproxPercentileCont {
args: AccumulatorArgs,
) -> Result<ApproxPercentileAccumulator> {
let percentile = validate_input_percentile_expr(&args.exprs[1])?;
+
+ let is_descending = args
+ .ordering_req
+ .first()
+ .map(|sort_expr| sort_expr.options.descending)
+ .unwrap_or(false);
+
+ let percentile = if is_descending {
+ 1.0 - percentile
+ } else {
+ percentile
+ };
Review Comment:
I used floating point subtraction instead of actual sorting in reverse order, for conciseness.
**_If any slight floating point difference is not permitted (even if this branch passed the tests), please let me know._**
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1976291064
##########
datafusion/expr/src/udaf.rs:
##########
@@ -845,6 +861,19 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
ScalarValue::try_from(data_type)
}
+ /// If this function supports `[IGNORE NULLS | RESPECT NULLS]` clause, return true
+ /// If the function does not, return false
+ /// Otherwise return None (the default)
+ fn supports_null_handling_clause(&self) -> Option<bool> {
+ None
+ }
+
+ /// If this function is ordered-set aggregate function, return true
+ /// If the function is not, return false
+ fn is_ordered_set_aggregate(&self) -> Option<bool> {
+ None
Review Comment:
I agree. I have applied it to the 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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1976304218
##########
datafusion/functions-aggregate/src/approx_percentile_cont.rs:
##########
@@ -51,29 +52,43 @@ create_func!(ApproxPercentileCont, approx_percentile_cont_udaf);
/// Computes the approximate percentile continuous of a set of numbers
pub fn approx_percentile_cont(
- expression: Expr,
+ within_group: Option<Vec<Sort>>,
Review Comment:
> Separately, why is this Option<Vec<Sort>>? As I understand it approx_percentile_cont must always have a WITHIN GROUP clause with a single ordering expression, so I would expect this to just be Sort to reflect that.
I removed `Option`, because `Sort` is necessary for ordered set aggregate function as you mentioned.
And also removed `Vec`.
(cf. I initially define it as `Vec<Sort>`, because there are some aggregate functions which supports multiple ordering expression in `WITHIN GROUP` clause. However, I have made sure at this time that even those DB engine (ex. Oracle, SQL Server) uses only a single ordering expression in `percentile_cont` and `approx_percentile`.)
Thank you for your guidance.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1972603943
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -146,6 +146,10 @@ pub struct SessionState {
scalar_functions: HashMap<String, Arc<ScalarUDF>>,
/// Aggregate functions registered in the context
aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
+ /// Ordered-set aggregate functions registered in the context
+ ///
+ /// Note : ordered_set_aggregate_functions are a subset of aggregate_functions.
+ ordered_set_aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
Review Comment:
Another approach that could work for this is would be to include this information in the `AggregateUDF` itself so that they can all be stored in `aggregate_functions` together
There are already properties like `order_sensitivity`
https://github.com/apache/datafusion/blob/fb40506b3f8d90930d4bf2b8b00fca5a5eac11fe/datafusion/expr/src/udaf.rs#L268-L272
and `groups_accumulator_supported`
https://github.com/apache/datafusion/blob/fb40506b3f8d90930d4bf2b8b00fca5a5eac11fe/datafusion/expr/src/udaf.rs#L234-L237
that are incorporated directly into the UDAF definition.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1976134261
##########
datafusion/core/tests/dataframe/dataframe_functions.rs:
##########
@@ -360,14 +360,15 @@ async fn test_fn_approx_median() -> Result<()> {
#[tokio::test]
async fn test_fn_approx_percentile_cont() -> Result<()> {
- let expr = approx_percentile_cont(col("b"), lit(0.5), None);
+ let expr =
+ approx_percentile_cont(Some(vec![col("b").sort(true, false)]), lit(0.5), None);
let expected = [
- "+---------------------------------------------+",
- "| approx_percentile_cont(test.b,Float64(0.5)) |",
- "+---------------------------------------------+",
- "| 10 |",
- "+---------------------------------------------+",
+ "+----------------------------------------------------------------------------------+",
+ "| approx_percentile_cont(test.b,Float64(0.5)) WITHIN GROUP [test.b ASC NULLS LAST] |",
Review Comment:
This name looks wrong to me. Shouldn't the `test.b` argument be only in the WITHIN GROUP section like
```
approx_percentile_cont(Float64(0.5)) WITHIN GROUP [test.b ASC NULLS LAST]
```
##########
datafusion/expr/src/udaf.rs:
##########
@@ -845,6 +861,19 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
ScalarValue::try_from(data_type)
}
+ /// If this function supports `[IGNORE NULLS | RESPECT NULLS]` clause, return true
+ /// If the function does not, return false
+ /// Otherwise return None (the default)
+ fn supports_null_handling_clause(&self) -> Option<bool> {
+ None
+ }
+
+ /// If this function is ordered-set aggregate function, return true
+ /// If the function is not, return false
+ fn is_ordered_set_aggregate(&self) -> Option<bool> {
+ None
Review Comment:
Would it make sense for this to just return `true` if is an ordered-set aggregate function and `false` otherwise and avoid the Option entirely?
##########
datafusion/expr/src/udaf.rs:
##########
@@ -845,6 +861,19 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
ScalarValue::try_from(data_type)
}
+ /// If this function supports `[IGNORE NULLS | RESPECT NULLS]` clause, return true
+ /// If the function does not, return false
+ /// Otherwise return None (the default)
+ fn supports_null_handling_clause(&self) -> Option<bool> {
+ None
+ }
Review Comment:
Is this something we need? From what I know, there aren't any aggregate functions that have options for null handling. At the moment, the 2 overrides you have of this both return `Some(false)`, which is what I would consider the default value anyways.
Speaking of which, if we do need this, do we need to return an `Optional<bool>` or could we just return `bool` directly?
##########
datafusion/functions-aggregate/src/approx_percentile_cont.rs:
##########
@@ -51,29 +52,43 @@ create_func!(ApproxPercentileCont, approx_percentile_cont_udaf);
/// Computes the approximate percentile continuous of a set of numbers
pub fn approx_percentile_cont(
- expression: Expr,
+ within_group: Option<Vec<Sort>>,
Review Comment:
Good call going from `Expr` to `Sort` here, as this lets us capture ASC/DESC which does appear to be valid in some engines (eg. [SQLServer](https://learn.microsoft.com/en-us/sql/t-sql/functions/approx-percentile-cont-transact-sql?view=sql-server-ver16)). It would probably be good to add a test for this if there isn't already one.
Separately, why is this `Option<Vec<Sort>>`? As I understand it `approx_percentile_cont` must always have a WITHIN GROUP clause with a single ordering expression, so I would expect this to just be `Sort` to reflect that.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1980817866
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -349,15 +365,49 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
} else {
// User defined aggregate functions (UDAF) have precedence in case it has the same name as a scalar built-in function
if let Some(fm) = self.context_provider.get_aggregate_meta(&name) {
- let order_by = self.order_by_to_sort_expr(
- order_by,
- schema,
- planner_context,
- true,
- None,
- )?;
- let order_by = (!order_by.is_empty()).then_some(order_by);
- let args = self.function_args_to_expr(args, schema, planner_context)?;
+ if fm.is_ordered_set_aggregate() && within_group.is_empty() {
+ return plan_err!("WITHIN GROUP clause is required when calling ordered set aggregate function({})", fm.name());
+ }
+
+ if null_treatment.is_some() && !fm.supports_null_handling_clause() {
+ return plan_err!(
+ "[IGNORE | RESPECT] NULLS are not permitted for {}",
+ fm.name()
+ );
+ }
Review Comment:
> One big question we might need to answer before merging is if we need a migration strategy for this. Because we now require WITHIN GROUP for these functions, any users who have queries stored outside of DataFusion will experience breakages that they can't work around. If we want to provide a migration path, we may need to support having both forms of calling these functions, as in
>
> SELECT approx_percentile_cont(column_name, 0.75, 100) FROM table_name;
> SELECT approx_percentile_cont(0.75, 100) WITHIN GROUP (ORDER BY column_name) FROM table_name;
>
> for at least 1 release so folks can migrate their queries.
This is one of the biggest concerns when I started to work on this feature.
If the community decides the migration strategy like that, then I will make both syntax supported.
Also, I will file an issue to track the plan so that the current syntax can be excluded as scheduled. (if I am authorized to do so)
--
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] Support within group syntax for existing aggregate functions [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2603471606
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1938219937
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -336,7 +369,28 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
None,
)?;
let order_by = (!order_by.is_empty()).then_some(order_by);
- let args = self.function_args_to_expr(args, schema, planner_context)?;
+ let mut args =
+ self.function_args_to_expr(args, schema, planner_context)?;
+
+ let within_group = self.order_by_to_sort_expr(
+ within_group,
+ schema,
+ planner_context,
+ false,
+ None,
+ )?;
+
+ // add target column expression in within group clause to function arguments
Review Comment:
The `Signature` of `ApproxPercentileCont` and `ApproxPercentileContWithWeight` are not changed. Even though the `direct argument (percentile)` is distinguished from the `aggregated argument (inside WITHIN GROUP clause)` when calling function, those arguments need to be handled as a whole during planning, in my opinion. That is because ordered set aggregate functions are not working only with `direct argument (percentile)`, without `WITHIN GROUP` clause.
Therefore, the expression in `WITHIN GROUP` clause is added into function arguments while converting `sql expr` to `logical expr`.
**_If this approach doesn't seem like a good idea, please let me know._**
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2644417031
Whenever the main branch is merged, conflicts can arise in certain test files that I also modified in this pull request. I have resolved these conflicts immediately after they occurred, but they might happen again as the main branch will continue to be merged.
From now on, I will fix conflicts after the review is completed or when I'm asked to do so.
fyi) I checked that workflows passed before the conflicts occurred.
--
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] Support within group syntax for existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1929539094
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -77,36 +77,38 @@ SELECT approx_distinct(c9) count_c9, approx_distinct(cast(c9 as varchar)) count_
# csv_query_approx_percentile_cont_with_weight
statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Utf8, Int8, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont_with_weight(c1, c2, 0.95) FROM aggregate_test_100
+SELECT approx_percentile_cont_with_weight(c2, 0.95) WITHIN GROUP (ORDER BY c1) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Int16, Utf8, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont_with_weight(c3, c1, 0.95) FROM aggregate_test_100
+SELECT approx_percentile_cont_with_weight(c1, 0.95) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Int16, Int8, Utf8\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont_with_weight(c3, c2, c1) FROM aggregate_test_100
+SELECT approx_percentile_cont_with_weight(c2, c1) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
# csv_query_approx_percentile_cont_with_histogram_bins
statement error DataFusion error: External error: This feature is not implemented: Tdigest max_size value for 'APPROX_PERCENTILE_CONT' must be UInt > 0 literal \(got data type Int64\)\.
-SELECT c1, approx_percentile_cont(c3, 0.95, -1000) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
+SELECT c1, approx_percentile_cont(0.95, -1000) WITHIN GROUP (ORDER BY c3) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Int16, Float64, Utf8\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont(c3, 0.95, c1) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, c1) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Int16, Float64, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont(c3, 0.95, 111.1) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, 111.1) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Float64, Float64, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont(c12, 0.95, 111.1) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, 111.1) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
statement error DataFusion error: This feature is not implemented: Percentile value for 'APPROX_PERCENTILE_CONT' must be a literal
-SELECT approx_percentile_cont(c12, c12) FROM aggregate_test_100
+SELECT approx_percentile_cont(c12) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
statement error DataFusion error: This feature is not implemented: Tdigest max_size value for 'APPROX_PERCENTILE_CONT' must be a literal
-SELECT approx_percentile_cont(c12, 0.95, c5) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, c5) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
# Not supported over sliding windows
query error This feature is not implemented: Aggregate can not be used as a sliding accumulator because `retract_batch` is not implemented
Review Comment:
```
External error: query is expected to fail with error:
(regex) This feature is not implemented: Aggregate can not be used as a sliding accumulator because `retract_batch` is not implemented
but got error:
DataFusion error: Error during planning: Error during planning: Coercion from [Float64] to the signature OneOf([Exact([Int8, Float64]), Exact([Int8, Float64, Int8]), Exact([Int8, Float64, Int16]), Exact([Int8, Float64, Int32]), Exact([Int8, Float64, Int64]), Exact([Int8, Float64, UInt8]), Exact([Int8, Float64, UInt16]), Exact([Int8, Float64, UInt32]), Exact([Int8, Float64, UInt64]), Exact([Int16, Float64]), Exact([Int16, Float64, Int8]), Exact([Int16, Float64, Int16]), Exact([Int16, Float64, Int32]), Exact([Int16, Float64, Int64]), Exact([Int16, Float64, UInt8]), Exact([Int16, Float64, UInt16]), Exact([Int16, Float64, UInt32]), Exact([Int16, Float64, UInt64]), Exact([Int32, Float64]), Exact([Int32, Float64, Int8]), Exact([Int32, Float64, Int16]), Exact([Int32, Float64, Int32]), Exact([Int32, Float64, Int64]), Exact([Int32, Float64, UInt8]), Exact([Int32, Float64, UInt16]), Exact([Int32, Float64, UInt32]), Exact([Int32, Float64, UInt64]), Exact([Int64, Float64]), Exact([Int
64, Float64, Int8]), Exact([Int64, Float64, Int16]), Exact([Int64, Float64, Int32]), Exact([Int64, Float64, Int64]), Exact([Int64, Float64, UInt8]), Exact([Int64, Float64, UInt16]), Exact([Int64, Float64, UInt32]), Exact([Int64, Float64, UInt64]), Exact([UInt8, Float64]), Exact([UInt8, Float64, Int8]), Exact([UInt8, Float64, Int16]), Exact([UInt8, Float64, Int32]), Exact([UInt8, Float64, Int64]), Exact([UInt8, Float64, UInt8]), Exact([UInt8, Float64, UInt16]), Exact([UInt8, Float64, UInt32]), Exact([UInt8, Float64, UInt64]), Exact([UInt16, Float64]), Exact([UInt16, Float64, Int8]), Exact([UInt16, Float64, Int16]), Exact([UInt16, Float64, Int32]), Exact([UInt16, Float64, Int64]), Exact([UInt16, Float64, UInt8]), Exact([UInt16, Float64, UInt16]), Exact([UInt16, Float64, UInt32]), Exact([UInt16, Float64, UInt64]), Exact([UInt32, Float64]), Exact([UInt32, Float64, Int8]), Exact([UInt32, Float64, Int16]), Exact([UInt32, Float64, Int32]), Exact([UInt32, Float64, Int64]), Exact([UInt32, Fl
oat64, UInt8]), Exact([UInt32, Float64, UInt16]), Exact([UInt32, Float64, UInt32]), Exact([UInt32, Float64, UInt64]), Exact([UInt64, Float64]), Exact([UInt64, Float64, Int8]), Exact([UInt64, Float64, Int16]), Exact([UInt64, Float64, Int32]), Exact([UInt64, Float64, Int64]), Exact([UInt64, Float64, UInt8]), Exact([UInt64, Float64, UInt16]), Exact([UInt64, Float64, UInt32]), Exact([UInt64, Float64, UInt64]), Exact([Float32, Float64]), Exact([Float32, Float64, Int8]), Exact([Float32, Float64, Int16]), Exact([Float32, Float64, Int32]), Exact([Float32, Float64, Int64]), Exact([Float32, Float64, UInt8]), Exact([Float32, Float64, UInt16]), Exact([Float32, Float64, UInt32]), Exact([Float32, Float64, UInt64]), Exact([Float64, Float64]), Exact([Float64, Float64, Int8]), Exact([Float64, Float64, Int16]), Exact([Float64, Float64, Int32]), Exact([Float64, Float64, Int64]), Exact([Float64, Float64, UInt8]), Exact([Float64, Float64, UInt16]), Exact([Float64, Float64, UInt32]), Exact([Float64, Floa
t64, UInt64])]) failed. No function matches the given name and argument types 'approx_percentile_cont(Float64)'. You might need to add explicit type casts.
Candidate functions:
approx_percentile_cont(Int8, Float64)
approx_percentile_cont(Int8, Float64, Int8)
approx_percentile_cont(Int8, Float64, Int16)
approx_percentile_cont(Int8, Float64, Int32)
approx_percentile_cont(Int8, Float64, Int64)
approx_percentile_cont(Int8, Float64, UInt8)
approx_percentile_cont(Int8, Float64, UInt16)
approx_percentile_cont(Int8, Float64, UInt32)
approx_percentile_cont(Int8, Float64, UInt64)
approx_percentile_cont(Int16, Float64)
approx_percentile_cont(Int16, Float64, Int8)
approx_percentile_cont(Int16, Float64, Int16)
approx_percentile_cont(Int16, Float64, Int32)
approx_percentile_cont(Int16, Float64, Int64)
approx_percentile_cont(Int16, Float64, UInt8)
approx_percentile_cont(Int16, Float64, UInt16)
approx_percentile_cont(Int16, Float64, UInt32)
approx_percentile_cont(Int16, Float64, UInt64)
approx_percentile_cont(Int32, Float64)
approx_percentile_cont(Int32, Float64, Int8)
approx_percentile_cont(Int32, Float64, Int16)
approx_percentile_cont(Int32, Float64, Int32)
approx_percentile_cont(Int32, Float64, Int64)
approx_percentile_cont(Int32, Float64, UInt8)
approx_percentile_cont(Int32, Float64, UInt16)
approx_percentile_cont(Int32, Float64, UInt32)
approx_percentile_cont(Int32, Float64, UInt64)
approx_percentile_cont(Int64, Float64)
approx_percentile_cont(Int64, Float64, Int8)
approx_percentile_cont(Int64, Float64, Int16)
approx_percentile_cont(Int64, Float64, Int32)
approx_percentile_cont(Int64, Float64, Int64)
approx_percentile_cont(Int64, Float64, UInt8)
approx_percentile_cont(Int64, Float64, UInt16)
approx_percentile_cont(Int64, Float64, UInt32)
approx_percentile_cont(Int64, Float64, UInt64)
approx_percentile_cont(UInt8, Float64)
approx_percentile_cont(UInt8, Float64, Int8)
approx_percentile_cont(UInt8, Float64, Int16)
approx_percentile_cont(UInt8, Float64, Int32)
approx_percentile_cont(UInt8, Float64, Int64)
approx_percentile_cont(UInt8, Float64, UInt8)
approx_percentile_cont(UInt8, Float64, UInt16)
approx_percentile_cont(UInt8, Float64, UInt32)
approx_percentile_cont(UInt8, Float64, UInt64)
approx_percentile_cont(UInt16, Float64)
approx_percentile_cont(UInt16, Float64, Int8)
approx_percentile_cont(UInt16, Float64, Int16)
approx_percentile_cont(UInt16, Float64, Int32)
approx_percentile_cont(UInt16, Float64, Int64)
approx_percentile_cont(UInt16, Float64, UInt8)
approx_percentile_cont(UInt16, Float64, UInt16)
approx_percentile_cont(UInt16, Float64, UInt32)
approx_percentile_cont(UInt16, Float64, UInt64)
approx_percentile_cont(UInt32, Float64)
approx_percentile_cont(UInt32, Float64, Int8)
approx_percentile_cont(UInt32, Float64, Int16)
approx_percentile_cont(UInt32, Float64, Int32)
approx_percentile_cont(UInt32, Float64, Int64)
approx_percentile_cont(UInt32, Float64, UInt8)
approx_percentile_cont(UInt32, Float64, UInt16)
approx_percentile_cont(UInt32, Float64, UInt32)
approx_percentile_cont(UInt32, Float64, UInt64)
approx_percentile_cont(UInt64, Float64)
approx_percentile_cont(UInt64, Float64, Int8)
approx_percentile_cont(UInt64, Float64, Int16)
approx_percentile_cont(UInt64, Float64, Int32)
approx_percentile_cont(UInt64, Float64, Int64)
approx_percentile_cont(UInt64, Float64, UInt8)
approx_percentile_cont(UInt64, Float64, UInt16)
approx_percentile_cont(UInt64, Float64, UInt32)
approx_percentile_cont(UInt64, Float64, UInt64)
approx_percentile_cont(Float32, Float64)
approx_percentile_cont(Float32, Float64, Int8)
approx_percentile_cont(Float32, Float64, Int16)
approx_percentile_cont(Float32, Float64, Int32)
approx_percentile_cont(Float32, Float64, Int64)
approx_percentile_cont(Float32, Float64, UInt8)
approx_percentile_cont(Float32, Float64, UInt16)
approx_percentile_cont(Float32, Float64, UInt32)
approx_percentile_cont(Float32, Float64, UInt64)
approx_percentile_cont(Float64, Float64)
approx_percentile_cont(Float64, Float64, Int8)
approx_percentile_cont(Float64, Float64, Int16)
approx_percentile_cont(Float64, Float64, Int32)
approx_percentile_cont(Float64, Float64, Int64)
approx_percentile_cont(Float64, Float64, UInt8)
approx_percentile_cont(Float64, Float64, UInt16)
approx_percentile_cont(Float64, Float64, UInt32)
approx_percentile_cont(Float64, Float64, UInt64)
[SQL] SELECT approx_percentile_cont(0.5)
WITHIN GROUP (ORDER BY c3)
OVER (ROWS BETWEEN 4 PRECEDING AND CURRENT ROW)
FROM aggregate_test_100
at test_files/aggregate.slt:108
```
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1938219937
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -336,7 +369,28 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
None,
)?;
let order_by = (!order_by.is_empty()).then_some(order_by);
- let args = self.function_args_to_expr(args, schema, planner_context)?;
+ let mut args =
+ self.function_args_to_expr(args, schema, planner_context)?;
+
+ let within_group = self.order_by_to_sort_expr(
+ within_group,
+ schema,
+ planner_context,
+ false,
+ None,
+ )?;
+
+ // add target column expression in within group clause to function arguments
Review Comment:
The `Signature` of `ApproxPercentileCont` and `ApproxPercentileContWithWeight` are not changed. Even though the `direct argument (percentile)` is distinguished from the `aggregated argument (inside WITHIN GROUP clause)` when calling function, in my opinion, those arguments need to be handled as a whole during planning.
Therefore, the expression in `WITHIN GROUP` clause is added into function arguments while converting `sql expr` to `logical expr`.
**_If this approach doesn't seem like a good idea, please let me know._**
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1938230292
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -146,6 +146,10 @@ pub struct SessionState {
scalar_functions: HashMap<String, Arc<ScalarUDF>>,
/// Aggregate functions registered in the context
aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
+ /// Ordered-set aggregate functions registered in the context
+ ///
+ /// Note : ordered_set_aggregate_functions are a subset of aggregate_functions.
+ ordered_set_aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
Review Comment:
There are certain cases when ordered set aggregate functions are needed to be recognized.
1. [`pub(super) fn sql_function_to_expr`](https://github.com/apache/datafusion/pull/13511/files#diff-e4760f867708d4ecd16dd4f38161a39d330c8d55f170e059aaba07317a07e42aR347-R354)
2. [`pub async fn from_substrait_agg_func`](https://github.com/apache/datafusion/pull/13511/files#diff-474e53672159d74dae38992a914a74eba81b8350ebe161f11d755f06414ed7b4R1955-R1959)
There is certainly a way to avoid using sessions in those cases. However, I am concerned that doing so might fragment the code.
**_If adding a new `ordered_set_aggregate_functions` field in the `SessionState` doesn't seem like a good idea, please let me know._**
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1976293270
##########
datafusion/expr/src/udaf.rs:
##########
@@ -845,6 +861,19 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
ScalarValue::try_from(data_type)
}
+ /// If this function supports `[IGNORE NULLS | RESPECT NULLS]` clause, return true
+ /// If the function does not, return false
+ /// Otherwise return None (the default)
+ fn supports_null_handling_clause(&self) -> Option<bool> {
+ None
+ }
Review Comment:
There are [some aggregate functions using null handling](https://github.com/apache/datafusion/blob/main/datafusion/sqllogictest/test_files/aggregate.slt#L5866-L5874) in current datafusion.
(cf. If this is something we need to discuss/fix, then I can make another git issue. Or, I can choose better option if there is. I left this comment because I am not 100% sure about the SQL standard.)
And I refactored the function to just return `bool`.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #13511:
URL: https://github.com/apache/datafusion/pull/13511
--
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] Support within group syntax for existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2603484747
> Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
Task is done, and I will commit changes and write comments this week.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2823913238
@jayzhan211 Thank you for reviewing!
However, I have one concern.
Is it okay to merge this PR right away, considering https://github.com/apache/datafusion/pull/13511#pullrequestreview-2658551401 ?
>
> One big question we might need to answer before merging is if we need a migration strategy for this. Because we now require WITHIN GROUP for these functions, any users who have queries stored outside of DataFusion will experience breakages that they can't work around. If we want to provide a migration path, we may need to support having both forms of calling these functions, as in
>
> ```
> SELECT approx_percentile_cont(column_name, 0.75, 100) FROM table_name;
> SELECT approx_percentile_cont(0.75, 100) WITHIN GROUP (ORDER BY column_name) FROM table_name;
> ```
>
> for at least 1 release so folks can migrate their queries.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1938224820
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -336,7 +369,28 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
None,
)?;
let order_by = (!order_by.is_empty()).then_some(order_by);
- let args = self.function_args_to_expr(args, schema, planner_context)?;
+ let mut args =
+ self.function_args_to_expr(args, schema, planner_context)?;
+
+ let within_group = self.order_by_to_sort_expr(
+ within_group,
+ schema,
+ planner_context,
+ false,
+ None,
+ )?;
+
+ // add target column expression in within group clause to function arguments
Review Comment:
The `Signature` of `ApproxPercentileCont` and `ApproxPercentileContWithWeight` are not changed. Even though the `direct argument (percentile)` is distinguished from the `aggregated argument (inside WITHIN GROUP clause)` when calling function, those arguments need to be handled as a whole during planning, in my opinion. That is because ordered set aggregate functions are not working only with `direct argument (percentile)`, without `WITHIN GROUP` clause.
Therefore, the expression in `WITHIN GROUP` clause is added into function arguments while converting `sql expr` to `logical expr`.
**_If this approach doesn't seem like a good idea, please let me know._**
##########
datafusion/functions-aggregate/src/approx_percentile_cont.rs:
##########
@@ -131,6 +147,19 @@ impl ApproxPercentileCont {
args: AccumulatorArgs,
) -> Result<ApproxPercentileAccumulator> {
let percentile = validate_input_percentile_expr(&args.exprs[1])?;
+
+ let is_descending = args
+ .ordering_req
+ .first()
+ .map(|sort_expr| sort_expr.options.descending)
+ .unwrap_or(false);
+
+ let percentile = if is_descending {
Review Comment:
I used floating point subtraction instead of actual sorting in reverse order, for conciseness.
**_If any slight floating point difference is not permitted (even if this branch passed the tests), please let me know._**
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2639739608
I looked over the code, it's clean and well written. I can't speak for the actual functionality here as it's not in my area of expertise. You need to regenerate the docs again though - ./dev/update_function_docs.sh is the script that will do that for 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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1976290842
##########
datafusion/core/tests/dataframe/dataframe_functions.rs:
##########
@@ -360,14 +360,15 @@ async fn test_fn_approx_median() -> Result<()> {
#[tokio::test]
async fn test_fn_approx_percentile_cont() -> Result<()> {
- let expr = approx_percentile_cont(col("b"), lit(0.5), None);
+ let expr =
+ approx_percentile_cont(Some(vec![col("b").sort(true, false)]), lit(0.5), None);
let expected = [
- "+---------------------------------------------+",
- "| approx_percentile_cont(test.b,Float64(0.5)) |",
- "+---------------------------------------------+",
- "| 10 |",
- "+---------------------------------------------+",
+ "+----------------------------------------------------------------------------------+",
+ "| approx_percentile_cont(test.b,Float64(0.5)) WITHIN GROUP [test.b ASC NULLS LAST] |",
Review Comment:
Thank you for identifying that. I have refactored the 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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1980829367
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -349,15 +365,49 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
} else {
// User defined aggregate functions (UDAF) have precedence in case it has the same name as a scalar built-in function
if let Some(fm) = self.context_provider.get_aggregate_meta(&name) {
- let order_by = self.order_by_to_sort_expr(
- order_by,
- schema,
- planner_context,
- true,
- None,
- )?;
- let order_by = (!order_by.is_empty()).then_some(order_by);
- let args = self.function_args_to_expr(args, schema, planner_context)?;
+ if fm.is_ordered_set_aggregate() && within_group.is_empty() {
+ return plan_err!("WITHIN GROUP clause is required when calling ordered set aggregate function({})", fm.name());
+ }
+
+ if null_treatment.is_some() && !fm.supports_null_handling_clause() {
+ return plan_err!(
+ "[IGNORE | RESPECT] NULLS are not permitted for {}",
+ fm.name()
+ );
+ }
Review Comment:
cf) I have applied all reviews that you tagged 'minor', since I was also convinced.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-3140896378
FYI we found several of our users were using the old syntax as well. I propose supporting both syntaxes
- https://github.com/apache/datafusion/issues/16955
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2825855767
I think support both query would be confusing, if we plan to end up support the new syntax at the end, it is better not to keep the old syntax
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2823874921
Thanks @Garamda and @vbarua!
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2823637363
I will send the conflict fix
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2636004482
> I will check if I can add more new test cases. (Additionally, I had no option but to modify some existing test cases; otherwise, they would fail due to the changed syntax)
Oh, I see. I thought the clause was optional, but you require it now
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1972589737
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -1619,13 +1620,20 @@ pub fn create_aggregate_expr_with_name_and_maybe_filter(
== NullTreatment::IgnoreNulls;
let (agg_expr, filter, order_by) = {
- let physical_sort_exprs = match order_by {
- Some(exprs) => Some(create_physical_sort_exprs(
- exprs,
+ let physical_sort_exprs = match within_group {
+ Some(within_group) => Some(create_physical_sort_exprs(
+ within_group,
logical_input_schema,
execution_props,
)?),
- None => None,
+ None => match order_by {
+ Some(order_by) => Some(create_physical_sort_exprs(
+ order_by,
+ logical_input_schema,
+ execution_props,
+ )?),
+ None => None,
+ },
Review Comment:
As an example of what I mean by the fact `within_groups` is redundant with `order_by` in the internal representation, here they both end up being processed identically.
##########
datafusion/expr/src/expr.rs:
##########
@@ -724,6 +727,7 @@ impl AggregateFunction {
filter,
order_by,
null_treatment,
+ within_group,
Review Comment:
At the SQL level, it makes sense to distinguish between WITHIN GROUP in something like
```sql
percentile_cont(fraction) WITHIN GROUP (ORDER BY sort_expression)
```
and ORDER BY in something like
```sql
array_agg ( anynonarray ORDER BY input_sort_columns )
```
but in the internal plan tree it feels redundant to track `order_by` and `within_group` separately. The `WITHIN GROUP (ORDER BY ...)` syntax is just a different way of setting the `order_by`.
I think this PR would be vastly simplified if you collapsed `within_group` into `order_by` in the internal tree, so that the only part of the code that cares about the difference is the SQL layer.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1975605829
##########
datafusion/expr/src/expr.rs:
##########
@@ -724,6 +727,7 @@ impl AggregateFunction {
filter,
order_by,
null_treatment,
+ within_group,
Review Comment:
@vbarua I appreciate your elaborate review.
Actually, 'where to convert WITHIN GROUP into ORDER BY' was one of the biggest things for me to consider.
I did not have clear standards when I started to work on this PR.
Your guide is right, so I refactored codes.
Would you review it?
I appreciate it again.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1938228189
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -336,7 +369,28 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
None,
)?;
let order_by = (!order_by.is_empty()).then_some(order_by);
- let args = self.function_args_to_expr(args, schema, planner_context)?;
+ let mut args =
+ self.function_args_to_expr(args, schema, planner_context)?;
+
+ let within_group = self.order_by_to_sort_expr(
+ within_group,
+ schema,
+ planner_context,
+ false,
+ None,
+ )?;
+
+ // add target column expression in within group clause to function arguments
+ if !within_group.is_empty() {
+ args = within_group
+ .iter()
+ .map(|sort| sort.expr.clone())
+ .chain(args)
+ .collect::<Vec<_>>();
+ }
Review Comment:
The `Signature` of `ApproxPercentileCont` and `ApproxPercentileContWithWeight` are not changed. Even though the `direct argument (percentile)` is distinguished from the `aggregated argument (inside WITHIN GROUP clause)` when calling function, those arguments need to be handled as a whole during planning, in my opinion. That is because ordered set aggregate functions are not working only with `direct argument (percentile)`, without `WITHIN GROUP` clause.
Therefore, the expression in `WITHIN GROUP` clause is added into function arguments while converting `sql expr` to `logical expr`.
**_If this approach doesn't seem like a good idea, please let me know._**
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2848017108
One of the extended clickbench queries also needed to be updated to the new syntax. I made a PR to do this here:
- https://github.com/apache/datafusion/pull/15929
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2641894768
Thank you @Omega359 for your comment.
`./dev/update_function_docs.sh` changes the order of function parameters in docs. This might differ from what users would naturally expect, so I was considering whether it's okay.
In any case, I'll apply the guidance provided by this shell script.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1938219937
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -336,7 +369,28 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
None,
)?;
let order_by = (!order_by.is_empty()).then_some(order_by);
- let args = self.function_args_to_expr(args, schema, planner_context)?;
+ let mut args =
+ self.function_args_to_expr(args, schema, planner_context)?;
+
+ let within_group = self.order_by_to_sort_expr(
+ within_group,
+ schema,
+ planner_context,
+ false,
+ None,
+ )?;
+
+ // add target column expression in within group clause to function arguments
Review Comment:
The `Signature` of `ApproxPercentileCont` and `ApproxPercentileContWithWeight` are not changed. Even though the `direct argument (percentile)` is distinguished from the `aggregated argument (inside WITHIN GROUP clause)` when calling function, those arguments need to be handled as a whole during planning, in my opinion. That is because ordered set aggregate functions are not working only with `direct argument (percentile)`, without `WITHIN GROUP` clause.
Therefore, the expression in `WITHIN GROUP` clause is added into function arguments while converting `sql expr` to `logical expr`.
**_If this approach doesn't seem like a good idea, please let me know._**
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1938224820
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -336,7 +369,28 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
None,
)?;
let order_by = (!order_by.is_empty()).then_some(order_by);
- let args = self.function_args_to_expr(args, schema, planner_context)?;
+ let mut args =
+ self.function_args_to_expr(args, schema, planner_context)?;
+
+ let within_group = self.order_by_to_sort_expr(
+ within_group,
+ schema,
+ planner_context,
+ false,
+ None,
+ )?;
+
+ // add target column expression in within group clause to function arguments
Review Comment:
The `Signature` of `ApproxPercentileCont` and `ApproxPercentileContWithWeight` are not changed. Even though the `direct argument (percentile)` is distinguished from the `aggregated argument (inside WITHIN GROUP clause)` when calling function, those arguments need to be handled as a whole during planning, in my opinion. That is because ordered set aggregate functions are not working only with `direct argument (percentile)`, without `WITHIN GROUP` clause.
Therefore, the expression in `WITHIN GROUP` clause is added into function arguments while converting `sql expr` to `logical expr`.
**_If this approach doesn't seem like a good idea, please let me know._**
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2628876789
for @jonathanc-n
- https://github.com/apache/datafusion/issues/12824#issuecomment-2401659938
- https://github.com/apache/datafusion/issues/12824#issuecomment-2408179355
Initially, I was supposed to work on step 1, and then hand it over to you for step 2.
However, I couldn't stop midway. I realized it was very difficult to separate step 1 and 2 in the middle.
I have implemented both step 1 & 2 in this pull request.
I apologize for not communicating about this earlier, and also apologize if you have been waiting to implement step 2.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2628877357
Next time, I will use a kebab-case branch name.
--
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] Support within group syntax for existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1929539094
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -77,36 +77,38 @@ SELECT approx_distinct(c9) count_c9, approx_distinct(cast(c9 as varchar)) count_
# csv_query_approx_percentile_cont_with_weight
statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Utf8, Int8, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont_with_weight(c1, c2, 0.95) FROM aggregate_test_100
+SELECT approx_percentile_cont_with_weight(c2, 0.95) WITHIN GROUP (ORDER BY c1) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Int16, Utf8, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont_with_weight(c3, c1, 0.95) FROM aggregate_test_100
+SELECT approx_percentile_cont_with_weight(c1, 0.95) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Int16, Int8, Utf8\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont_with_weight(c3, c2, c1) FROM aggregate_test_100
+SELECT approx_percentile_cont_with_weight(c2, c1) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
# csv_query_approx_percentile_cont_with_histogram_bins
statement error DataFusion error: External error: This feature is not implemented: Tdigest max_size value for 'APPROX_PERCENTILE_CONT' must be UInt > 0 literal \(got data type Int64\)\.
-SELECT c1, approx_percentile_cont(c3, 0.95, -1000) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
+SELECT c1, approx_percentile_cont(0.95, -1000) WITHIN GROUP (ORDER BY c3) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Int16, Float64, Utf8\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont(c3, 0.95, c1) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, c1) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Int16, Float64, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont(c3, 0.95, 111.1) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, 111.1) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Float64, Float64, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont(c12, 0.95, 111.1) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, 111.1) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
statement error DataFusion error: This feature is not implemented: Percentile value for 'APPROX_PERCENTILE_CONT' must be a literal
-SELECT approx_percentile_cont(c12, c12) FROM aggregate_test_100
+SELECT approx_percentile_cont(c12) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
statement error DataFusion error: This feature is not implemented: Tdigest max_size value for 'APPROX_PERCENTILE_CONT' must be a literal
-SELECT approx_percentile_cont(c12, 0.95, c5) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, c5) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
# Not supported over sliding windows
query error This feature is not implemented: Aggregate can not be used as a sliding accumulator because `retract_batch` is not implemented
Review Comment:
All test cases passed except for this one.
```
External error: query is expected to fail with error:
(regex) This feature is not implemented: Aggregate can not be used as a sliding accumulator because `retract_batch` is not implemented
but got error:
DataFusion error: Error during planning: Error during planning: Coercion from [Float64] to the signature OneOf([Exact([Int8, Float64]), Exact([Int8, Float64, Int8]), Exact([Int8, Float64, Int16]), Exact([Int8, Float64, Int32]), Exact([Int8, Float64, Int64]), Exact([Int8, Float64, UInt8]), Exact([Int8, Float64, UInt16]), Exact([Int8, Float64, UInt32]), Exact([Int8, Float64, UInt64]), Exact([Int16, Float64]), Exact([Int16, Float64, Int8]), Exact([Int16, Float64, Int16]), Exact([Int16, Float64, Int32]), Exact([Int16, Float64, Int64]), Exact([Int16, Float64, UInt8]), Exact([Int16, Float64, UInt16]), Exact([Int16, Float64, UInt32]), Exact([Int16, Float64, UInt64]), Exact([Int32, Float64]), Exact([Int32, Float64, Int8]), Exact([Int32, Float64, Int16]), Exact([Int32, Float64, Int32]), Exact([Int32, Float64, Int64]), Exact([Int32, Float64, UInt8]), Exact([Int32, Float64, UInt16]), Exact([Int32, Float64, UInt32]), Exact([Int32, Float64, UInt64]), Exact([Int64, Float64]), Exact([Int
64, Float64, Int8]), Exact([Int64, Float64, Int16]), Exact([Int64, Float64, Int32]), Exact([Int64, Float64, Int64]), Exact([Int64, Float64, UInt8]), Exact([Int64, Float64, UInt16]), Exact([Int64, Float64, UInt32]), Exact([Int64, Float64, UInt64]), Exact([UInt8, Float64]), Exact([UInt8, Float64, Int8]), Exact([UInt8, Float64, Int16]), Exact([UInt8, Float64, Int32]), Exact([UInt8, Float64, Int64]), Exact([UInt8, Float64, UInt8]), Exact([UInt8, Float64, UInt16]), Exact([UInt8, Float64, UInt32]), Exact([UInt8, Float64, UInt64]), Exact([UInt16, Float64]), Exact([UInt16, Float64, Int8]), Exact([UInt16, Float64, Int16]), Exact([UInt16, Float64, Int32]), Exact([UInt16, Float64, Int64]), Exact([UInt16, Float64, UInt8]), Exact([UInt16, Float64, UInt16]), Exact([UInt16, Float64, UInt32]), Exact([UInt16, Float64, UInt64]), Exact([UInt32, Float64]), Exact([UInt32, Float64, Int8]), Exact([UInt32, Float64, Int16]), Exact([UInt32, Float64, Int32]), Exact([UInt32, Float64, Int64]), Exact([UInt32, Fl
oat64, UInt8]), Exact([UInt32, Float64, UInt16]), Exact([UInt32, Float64, UInt32]), Exact([UInt32, Float64, UInt64]), Exact([UInt64, Float64]), Exact([UInt64, Float64, Int8]), Exact([UInt64, Float64, Int16]), Exact([UInt64, Float64, Int32]), Exact([UInt64, Float64, Int64]), Exact([UInt64, Float64, UInt8]), Exact([UInt64, Float64, UInt16]), Exact([UInt64, Float64, UInt32]), Exact([UInt64, Float64, UInt64]), Exact([Float32, Float64]), Exact([Float32, Float64, Int8]), Exact([Float32, Float64, Int16]), Exact([Float32, Float64, Int32]), Exact([Float32, Float64, Int64]), Exact([Float32, Float64, UInt8]), Exact([Float32, Float64, UInt16]), Exact([Float32, Float64, UInt32]), Exact([Float32, Float64, UInt64]), Exact([Float64, Float64]), Exact([Float64, Float64, Int8]), Exact([Float64, Float64, Int16]), Exact([Float64, Float64, Int32]), Exact([Float64, Float64, Int64]), Exact([Float64, Float64, UInt8]), Exact([Float64, Float64, UInt16]), Exact([Float64, Float64, UInt32]), Exact([Float64, Floa
t64, UInt64])]) failed. No function matches the given name and argument types 'approx_percentile_cont(Float64)'. You might need to add explicit type casts.
Candidate functions:
approx_percentile_cont(Int8, Float64)
approx_percentile_cont(Int8, Float64, Int8)
approx_percentile_cont(Int8, Float64, Int16)
approx_percentile_cont(Int8, Float64, Int32)
approx_percentile_cont(Int8, Float64, Int64)
approx_percentile_cont(Int8, Float64, UInt8)
approx_percentile_cont(Int8, Float64, UInt16)
approx_percentile_cont(Int8, Float64, UInt32)
approx_percentile_cont(Int8, Float64, UInt64)
approx_percentile_cont(Int16, Float64)
approx_percentile_cont(Int16, Float64, Int8)
approx_percentile_cont(Int16, Float64, Int16)
approx_percentile_cont(Int16, Float64, Int32)
approx_percentile_cont(Int16, Float64, Int64)
approx_percentile_cont(Int16, Float64, UInt8)
approx_percentile_cont(Int16, Float64, UInt16)
approx_percentile_cont(Int16, Float64, UInt32)
approx_percentile_cont(Int16, Float64, UInt64)
approx_percentile_cont(Int32, Float64)
approx_percentile_cont(Int32, Float64, Int8)
approx_percentile_cont(Int32, Float64, Int16)
approx_percentile_cont(Int32, Float64, Int32)
approx_percentile_cont(Int32, Float64, Int64)
approx_percentile_cont(Int32, Float64, UInt8)
approx_percentile_cont(Int32, Float64, UInt16)
approx_percentile_cont(Int32, Float64, UInt32)
approx_percentile_cont(Int32, Float64, UInt64)
approx_percentile_cont(Int64, Float64)
approx_percentile_cont(Int64, Float64, Int8)
approx_percentile_cont(Int64, Float64, Int16)
approx_percentile_cont(Int64, Float64, Int32)
approx_percentile_cont(Int64, Float64, Int64)
approx_percentile_cont(Int64, Float64, UInt8)
approx_percentile_cont(Int64, Float64, UInt16)
approx_percentile_cont(Int64, Float64, UInt32)
approx_percentile_cont(Int64, Float64, UInt64)
approx_percentile_cont(UInt8, Float64)
approx_percentile_cont(UInt8, Float64, Int8)
approx_percentile_cont(UInt8, Float64, Int16)
approx_percentile_cont(UInt8, Float64, Int32)
approx_percentile_cont(UInt8, Float64, Int64)
approx_percentile_cont(UInt8, Float64, UInt8)
approx_percentile_cont(UInt8, Float64, UInt16)
approx_percentile_cont(UInt8, Float64, UInt32)
approx_percentile_cont(UInt8, Float64, UInt64)
approx_percentile_cont(UInt16, Float64)
approx_percentile_cont(UInt16, Float64, Int8)
approx_percentile_cont(UInt16, Float64, Int16)
approx_percentile_cont(UInt16, Float64, Int32)
approx_percentile_cont(UInt16, Float64, Int64)
approx_percentile_cont(UInt16, Float64, UInt8)
approx_percentile_cont(UInt16, Float64, UInt16)
approx_percentile_cont(UInt16, Float64, UInt32)
approx_percentile_cont(UInt16, Float64, UInt64)
approx_percentile_cont(UInt32, Float64)
approx_percentile_cont(UInt32, Float64, Int8)
approx_percentile_cont(UInt32, Float64, Int16)
approx_percentile_cont(UInt32, Float64, Int32)
approx_percentile_cont(UInt32, Float64, Int64)
approx_percentile_cont(UInt32, Float64, UInt8)
approx_percentile_cont(UInt32, Float64, UInt16)
approx_percentile_cont(UInt32, Float64, UInt32)
approx_percentile_cont(UInt32, Float64, UInt64)
approx_percentile_cont(UInt64, Float64)
approx_percentile_cont(UInt64, Float64, Int8)
approx_percentile_cont(UInt64, Float64, Int16)
approx_percentile_cont(UInt64, Float64, Int32)
approx_percentile_cont(UInt64, Float64, Int64)
approx_percentile_cont(UInt64, Float64, UInt8)
approx_percentile_cont(UInt64, Float64, UInt16)
approx_percentile_cont(UInt64, Float64, UInt32)
approx_percentile_cont(UInt64, Float64, UInt64)
approx_percentile_cont(Float32, Float64)
approx_percentile_cont(Float32, Float64, Int8)
approx_percentile_cont(Float32, Float64, Int16)
approx_percentile_cont(Float32, Float64, Int32)
approx_percentile_cont(Float32, Float64, Int64)
approx_percentile_cont(Float32, Float64, UInt8)
approx_percentile_cont(Float32, Float64, UInt16)
approx_percentile_cont(Float32, Float64, UInt32)
approx_percentile_cont(Float32, Float64, UInt64)
approx_percentile_cont(Float64, Float64)
approx_percentile_cont(Float64, Float64, Int8)
approx_percentile_cont(Float64, Float64, Int16)
approx_percentile_cont(Float64, Float64, Int32)
approx_percentile_cont(Float64, Float64, Int64)
approx_percentile_cont(Float64, Float64, UInt8)
approx_percentile_cont(Float64, Float64, UInt16)
approx_percentile_cont(Float64, Float64, UInt32)
approx_percentile_cont(Float64, Float64, UInt64)
[SQL] SELECT approx_percentile_cont(0.5)
WITHIN GROUP (ORDER BY c3)
OVER (ROWS BETWEEN 4 PRECEDING AND CURRENT ROW)
FROM aggregate_test_100
at test_files/aggregate.slt:108
```
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2635495223
Thank you @berkaysynnada for your review, and for letting me know about my mistake (submodule changes). I have just reverted submodule changes.
I will check if I can add more new test cases.
(Additionally, I had no option but to modify some existing test cases; otherwise, they would fail due to the changed syntax)
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#issuecomment-2633502146
Thank you @Garamda, the code is well-written and has a good documentation. I couldn't fully think on the details of the implementation, but as an early review, I can suggest adding new tests instead of changing the existing ones, and you should revert the submodule changes.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1976293270
##########
datafusion/expr/src/udaf.rs:
##########
@@ -845,6 +861,19 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
ScalarValue::try_from(data_type)
}
+ /// If this function supports `[IGNORE NULLS | RESPECT NULLS]` clause, return true
+ /// If the function does not, return false
+ /// Otherwise return None (the default)
+ fn supports_null_handling_clause(&self) -> Option<bool> {
+ None
+ }
Review Comment:
There are [some aggregate functions using null handling](https://github.com/apache/datafusion/blob/main/datafusion/sqllogictest/test_files/aggregate.slt#L5866-L5874) in current datafusion.
(cf. If this is something we need to discuss/fix, then I can make another git issue. Or, I can refactor it too in this PR. I left this comment because I am not 100% sure about the SQL standard.)
And I refactored the function to just return `bool`.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1979981659
##########
datafusion/expr/src/expr.rs:
##########
@@ -295,6 +295,8 @@ pub enum Expr {
/// See also [`ExprFunctionExt`] to set these fields.
///
/// [`ExprFunctionExt`]: crate::expr_fn::ExprFunctionExt
+ ///
+ /// cf. `WITHIN GROUP` is converted to `ORDER BY` internally in `datafusion/sql/src/expr/function.rs`
Review Comment:
minor/opinionated: I'm not sure if it's worth mentioning this at all here. `WITHIN GROUP` is effectively an `ORDER BY` specified differently. This only matters at the SQL layer, and you handle and explain it there already.
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -349,15 +365,49 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
} else {
// User defined aggregate functions (UDAF) have precedence in case it has the same name as a scalar built-in function
if let Some(fm) = self.context_provider.get_aggregate_meta(&name) {
- let order_by = self.order_by_to_sort_expr(
- order_by,
- schema,
- planner_context,
- true,
- None,
- )?;
- let order_by = (!order_by.is_empty()).then_some(order_by);
- let args = self.function_args_to_expr(args, schema, planner_context)?;
+ if fm.is_ordered_set_aggregate() && within_group.is_empty() {
+ return plan_err!("WITHIN GROUP clause is required when calling ordered set aggregate function({})", fm.name());
+ }
+
+ if null_treatment.is_some() && !fm.supports_null_handling_clause() {
+ return plan_err!(
+ "[IGNORE | RESPECT] NULLS are not permitted for {}",
+ fm.name()
+ );
+ }
Review Comment:
Per my point about `[IGNORE | RESPECT] NULLS` being a property of _window functions_, I don't think we need this check here.
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -177,8 +180,14 @@ impl FunctionArgs {
}
}
- if !within_group.is_empty() {
- return not_impl_err!("WITHIN GROUP is not supported yet: {within_group:?}");
+ if !within_group.is_empty() && order_by.is_some() {
+ return plan_err!("ORDER BY clause is only permitted in WITHIN GROUP clause when a WITHIN GROUP is used");
+ }
+
+ if within_group.len() > 1 {
+ return not_impl_err!(
+ "Multiple column ordering in WITHIN GROUP clause is not supported"
Review Comment:
Minor wording suggestion
```
Only a single ordering expression is permitted in a WITHIN GROUP clause
```
which explicitly points users to what they should do, instead of telling them what they can't.
##########
datafusion/functions-aggregate/src/approx_percentile_cont.rs:
##########
@@ -51,29 +52,39 @@ create_func!(ApproxPercentileCont, approx_percentile_cont_udaf);
/// Computes the approximate percentile continuous of a set of numbers
pub fn approx_percentile_cont(
- expression: Expr,
+ within_group: Sort,
Review Comment:
minor/opinionated: I think `order_by` would be a clearer name for this, as the `WITHIN GROUP` is really just a wrapper around the `ORDER BY` clause.
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -177,8 +180,14 @@ impl FunctionArgs {
}
}
- if !within_group.is_empty() {
- return not_impl_err!("WITHIN GROUP is not supported yet: {within_group:?}");
+ if !within_group.is_empty() && order_by.is_some() {
+ return plan_err!("ORDER BY clause is only permitted in WITHIN GROUP clause when a WITHIN GROUP is used");
Review Comment:
minor: I just noticed that in the block above there is a check for duplicate order bys. I think it would be good to fold this into that check
```rust
FunctionArgumentClause::OrderBy(oby) => {
if order_by.is_some() { // can check for within group here
return not_impl_err!("Calling {name}: Duplicated ORDER BY clause in function arguments");
}
order_by = Some(oby);
}
```
to consolidate the handling into one place.
##########
datafusion/functions-aggregate/src/approx_percentile_cont.rs:
##########
@@ -131,6 +147,19 @@ impl ApproxPercentileCont {
args: AccumulatorArgs,
) -> Result<ApproxPercentileAccumulator> {
let percentile = validate_input_percentile_expr(&args.exprs[1])?;
+
+ let is_descending = args
+ .ordering_req
+ .first()
+ .map(|sort_expr| sort_expr.options.descending)
+ .unwrap_or(false);
+
+ let percentile = if is_descending {
+ 1.0 - percentile
+ } else {
+ percentile
+ };
Review Comment:
This seems reasonable to me, but I don't have that much experience on the execution side of things.
##########
datafusion/expr/src/udaf.rs:
##########
@@ -845,6 +861,19 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
ScalarValue::try_from(data_type)
}
+ /// If this function supports `[IGNORE NULLS | RESPECT NULLS]` clause, return true
+ /// If the function does not, return false
+ /// Otherwise return None (the default)
+ fn supports_null_handling_clause(&self) -> Option<bool> {
+ None
+ }
Review Comment:
This was smelling odd, so I dug a bit deeper. I think you've inadvertantly stumbled into something even weirder than you anticipated
The example you've linked is
```sql
SELECT FIRST_VALUE(column1) RESPECT NULLS FROM t;
```
which I don't think is a valid query because `first_value` _should not_ be an aggregate function, or at the very least the above query is not valid in most SQL dialects. `first_value` is actually a window function in other engines (eg. [Trino](https://trino.io/docs/current/functions/window.html#first_value), [Postgres](https://www.postgresql.org/docs/17/functions-window.html), [MySQL](https://dev.mysql.com/doc/refman/8.4/en/window-function-descriptions.html#function_first-value)).
If you try running something like
```sql
SELECT first_value(column1) FROM t;
```
against Postgres you get an error like
```
Query Error: window function first_value requires an OVER clause
```
[dbfiddle](https://www.db-fiddle.com/f/oMU1DDB2915nrHLKqUVj9a/0)
The `RESPECT NULLS | IGNORE NULLS` options is only a property of certain _window_ functions, hence we shouldn't need to track it for aggregate functions.
I'm going to file a ticket for the above.
--
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] Support WITHIN GROUP syntax to standardize certain existing aggregate functions [datafusion]
Posted by "Garamda (via GitHub)" <gi...@apache.org>.
Garamda commented on code in PR #13511:
URL: https://github.com/apache/datafusion/pull/13511#discussion_r1980826096
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -349,15 +365,49 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
} else {
// User defined aggregate functions (UDAF) have precedence in case it has the same name as a scalar built-in function
if let Some(fm) = self.context_provider.get_aggregate_meta(&name) {
- let order_by = self.order_by_to_sort_expr(
- order_by,
- schema,
- planner_context,
- true,
- None,
- )?;
- let order_by = (!order_by.is_empty()).then_some(order_by);
- let args = self.function_args_to_expr(args, schema, planner_context)?;
+ if fm.is_ordered_set_aggregate() && within_group.is_empty() {
+ return plan_err!("WITHIN GROUP clause is required when calling ordered set aggregate function({})", fm.name());
+ }
+
+ if null_treatment.is_some() && !fm.supports_null_handling_clause() {
+ return plan_err!(
+ "[IGNORE | RESPECT] NULLS are not permitted for {}",
+ fm.name()
+ );
+ }
Review Comment:
> This was smelling odd, so I dug a bit deeper. I think you've inadvertantly stumbled into something even weirder than you anticipated
> ...
> The RESPECT NULLS | IGNORE NULLS options is only a property of certain window functions, hence we shouldn't need to track it for aggregate functions.
>
> I'm going to file a ticket for the above.
> ...
> Per my point about [IGNORE | RESPECT] NULLS being a property of window functions, I don't think we need this check here.
I understood and agree with your guidance.
I will track what is decided in the issue you filed, and will remove some codes out after determination.
--
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