You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "cj-zhukov (via GitHub)" <gi...@apache.org> on 2026/02/18 12:28:56 UTC

[PR] Disallow order by within ordered-set aggregate functions argument lis… [datafusion]

cj-zhukov opened a new pull request, #20421:
URL: https://github.com/apache/datafusion/pull/20421

   …ts(#18281)
   
   ## 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 #https://github.com/apache/datafusion/issues/18281.
   
   ## 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] Disallow order by within ordered-set aggregate functions argument lists [datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#discussion_r2984167461


##########
datafusion/sql/src/expr/function.rs:
##########
@@ -867,6 +905,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         Ok((exprs, names))
     }
 
+    #[expect(dead_code)]

Review Comment:
   > Would you expect inline ORDER BY to be supported only for quantile_cont (and similar ordered-set aggregates), or should this be extended more generally to other aggregate functions as well?
   
   I am not sure -- I basically expect that datafusion would follow other implementations rather than creating our own syntax. In the case where previous versions of DataFusion supported syntax that is not supported by other systems, I would expect that syntax to keep working, though we don't have to extend it to other functions that didn't previously support 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] Disallow order by within ordered-set aggregate functions argument lists [datafusion]

Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#discussion_r2850652099


##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -129,6 +129,14 @@ CREATE TABLE group_median_table_nullable (
 # Error tests
 #######
 
+statement error ORDER BY must be specified using WITHIN GROUP
+select quantile_cont(col0, 0.75 order by col0)
+from values (1, 3), (2, 2), (3, 1) t(col0, col1);
+
+statement error ORDER BY must be specified using WITHIN GROUP

Review Comment:
   The query above currently succeeds on main
   
   ```sql
   select quantile_cont(col0, 0.75 order by col0)
   from values (1, 3), (2, 2), (3, 1) t(col0, col1);
   ```
   
   I was thinking this could be a breaking change, but I wonder how often these (ordered set aggregates) are currently used especially with an `order by` in the argument list like this; if we want to be fully explicit we can add an entry to the upgrade guide



-- 
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] Disallow order by within ordered-set aggregate functions argument lists [datafusion]

Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#issuecomment-3988809524

   > > Given that, I’m wondering if it would make sense to keep this PR focused and open a separate PR dedicated specifically to supporting both syntaxes.
   > 
   > Probably
   > 
   > I am just worried about disallowing syntax that was previously allowed as I worry it will cause downstream issues (we have hit stuff like this in the past at InfluxData)
   > 
   > From a customers perspective they are like "my query use to work but no longer does"
   > 
   > Unless we have some sort of more compelling story (like "it was getting the wrong answer and now you get an error which is better") it is hard to justify such changes
   
   Thanks for clarifying - that makes sense.
   I agree that preserving previously supported syntax is important for downstream users, especially to avoid unexpected regressions.
   
   I’ll update this PR to continue supporting both syntaxes rather than introducing a breaking change. I’ll follow up once I have a clean solution in place.


-- 
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] Disallow order by within ordered-set aggregate functions argument lists [datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #20421:
URL: https://github.com/apache/datafusion/pull/20421


-- 
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] Disallow order by within ordered-set aggregate functions argument lists [datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#discussion_r2933305431


##########
datafusion/sql/src/expr/function.rs:
##########
@@ -867,6 +905,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         Ok((exprs, names))
     }
 
+    #[expect(dead_code)]

Review Comment:
   I think this means the code is not used -- shouldn't we just remove it rather than making `#[expect(dead)]`?



-- 
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] Disallow order by within ordered-set aggregate functions argument lis… [datafusion]

Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#issuecomment-3920591723

   ### High-level overview
   This PR fixes incorrect handling of ordered-set aggregate syntax.
   
   Specifically:
   - Updated `datafusion/sql/src/expr/function.rs` to return a planning error when `ORDER BY` is used inline 
   - Added test coverage in `datafusion/sqllogictest/test_files/aggregate.slt` to ensure these invalid forms now return a syntax/planning error


-- 
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] Disallow order by within ordered-set aggregate functions argument lists [datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#issuecomment-3985377782

   > Given that, I’m wondering if it would make sense to keep this PR focused and open a separate PR dedicated specifically to supporting both syntaxes.
   
   Probably
   
   I am just worried about disallowing syntax that was previously allowed as I worry it will cause downstream issues (we have hit stuff like this in the past at InfluxData)
   
   From a customers perspective they are like "my query use to work but no longer does" 
   
   Unless we have some sort of more compelling story (like "it was getting the wrong answer and now you get an error which is better") it is hard to justify such 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] Disallow order by within ordered-set aggregate functions argument lists [datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#discussion_r2990514043


##########
datafusion/sql/src/expr/function.rs:
##########
@@ -867,6 +905,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         Ok((exprs, names))
     }
 
+    #[expect(dead_code)]

Review Comment:
   Thank you -- I am sorry for the delay / back and forth on this -- but we have been bitten downstream in the past when stuff that used to work stopped working



-- 
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] Disallow order by within ordered-set aggregate functions argument lists [datafusion]

Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#discussion_r2986032358


##########
datafusion/sql/src/expr/function.rs:
##########
@@ -867,6 +905,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         Ok((exprs, names))
     }
 
+    #[expect(dead_code)]

Review Comment:
   Thanks, that helps clarify the direction.
   
   I’ll keep the inline `ORDER BY` syntax only for cases where it was already supported. I’ll also add tests to ensure we preserve existing behavior while rejecting unsupported cases more clearly.



-- 
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] Disallow order by within ordered-set aggregate functions argument lists [datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#issuecomment-4180555155

   I merged up again to get a clean CI run --I'll plan to merge this PR once the CI passes


-- 
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] Disallow order by within ordered-set aggregate functions argument lists [datafusion]

Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#issuecomment-4004180101

   This PR now supports both syntaxes: 
   ```
   select quantile_cont(col0, 0.75 order by col0) from values (1, 3), (2, 2), (3, 1) t(col0, col1);
   select quantile_cont(0.75 order by col0) from values (1, 3), (2, 2), (3, 1) t(col0, col1);
   ```
   
   What do you think about supporting both syntaxes? Happy to adjust if there is a preference to keep only one form.


-- 
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] Disallow order by within ordered-set aggregate functions argument lists [datafusion]

Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#issuecomment-3990246536

   > Unless we have some sort of more compelling story (like "it was getting the wrong answer and now you get an error which is better") it is hard to justify such changes
   
   I believe this might be the case here? As far as I'm aware, the `order by` syntax here essentially does nothing, so if there is a query like:
   
   ```sql
   select quantile_cont(col0, 0.75 order by col0 desc)
   from values (1, 3), (2, 2), (3, 1) t(col0, col1);
   ```
   
   Then it is would actually be computed as ascending (the default since `within group` isn't specified). Fixing to support `order by` is also a good move, though that might be more subtle as any such queries would now return a different result 🤔 (i.e. harder to make users aware of this new behaviour)


-- 
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] Disallow order by within ordered-set aggregate functions argument lists [datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#discussion_r2854879367


##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -129,6 +129,14 @@ CREATE TABLE group_median_table_nullable (
 # Error tests
 #######
 
+statement error ORDER BY must be specified using WITHIN GROUP
+select quantile_cont(col0, 0.75 order by col0)
+from values (1, 3), (2, 2), (3, 1) t(col0, col1);
+
+statement error ORDER BY must be specified using WITHIN GROUP

Review Comment:
   I double checked and I agree with you
   ```sql
   andrewlamb@Andrews-MacBook-Pro-3:~/Software/influxdb_pro/ent$ datafusion-cli
   DataFusion CLI v52.1.0
   > select quantile_cont(col0, 0.75 order by col0)
   from values (1, 3), (2, 2), (3, 1) t(col0, col1);
   +-------------------------------------+
   | quantile_cont(t.col0,Float64(0.75)) |
   +-------------------------------------+
   | 2.5                                 |
   +-------------------------------------+
   1 row(s) fetched.
   Elapsed 0.002 seconds.
   ```
   
   In general, rather than disallow this syntax, is there some way we can support both syntaxes? I also have no idea how often they are used but if it is currently supported and produces the correct result, then I suspect at least some users would treat them starting to error as a regression 🤔 
   



-- 
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] Disallow order by within ordered-set aggregate functions argument lists [datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#discussion_r2849542704


##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -129,6 +129,14 @@ CREATE TABLE group_median_table_nullable (
 # Error tests
 #######
 
+statement error ORDER BY must be specified using WITHIN GROUP
+select quantile_cont(col0, 0.75 order by col0)
+from values (1, 3), (2, 2), (3, 1) t(col0, col1);
+
+statement error ORDER BY must be specified using WITHIN GROUP

Review Comment:
   I double checked that these queries fail on main too:
   
   ```sql
   > select quantile_cont(0.75 order by col0)
   from values (1, 3), (2, 2), (3, 1) t(col0, col1);
   Error during planning: Function 'percentile_cont' expects 2 arguments but received 1 No function matches the given name and argument types 'percentile_cont(Float64)'. You might need to add explicit type casts.
   	Candidate functions:
   	percentile_cont(expr: Coercion(TypeSignatureClass::Float, implicit_coercion=ImplicitCoercion([Numeric], default_type=Float64), percentile: Coercion(TypeSignatureClass::Native(LogicalType(Native(Float64), Float64)), implicit_coercion=ImplicitCoercion([Numeric], default_type=Float64))
   ```



-- 
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] Disallow order by within ordered-set aggregate functions argument lis… [datafusion]

Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#issuecomment-3920685441

   @Jefffrey Since you originally opened this issue, could you please take a look at the proposed refactor?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@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] Disallow order by within ordered-set aggregate functions argument lists [datafusion]

Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#discussion_r2973707951


##########
datafusion/sql/src/expr/function.rs:
##########
@@ -867,6 +905,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         Ok((exprs, names))
     }
 
+    #[expect(dead_code)]

Review Comment:
   I think I may be missing part of the intended scope, so I’d appreciate some clarification.
   
   It looks like we may need broader test coverage here - not only for `quantile_cont`, but also for other aggregate functions and similar usage patterns, to better define the expected behavior.
   
   Originally, this issue was about rejecting invalid `ORDER BY` usage inside aggregate argument lists earlier. My initial fix enforced that, but it introduced a breaking change, so I updated the implementation to support both syntaxes for `quantile_cont`. This works for the cases I’ve added tests for.
   
   Regarding your example:
   ```sql
   SELECT approx_percentile_cont_with_weight(1, 0.95 ORDER BY x)
   FROM (VALUES (1), (2), (3)) AS t(x);
   ```
   I didn’t consider this kind of usage across other aggregate functions when implementing the change, so I’m not fully sure what the expected behavior should be here.
   
   Would you expect inline `ORDER BY` to be supported only for `quantile_cont` (and similar ordered-set aggregates), or should this be extended more generally to other aggregate functions as well?



-- 
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