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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to