alamb commented on code in PR #16625:
URL: https://github.com/apache/datafusion/pull/16625#discussion_r2217293999


##########
datafusion/ffi/src/udaf/mod.rs:
##########
@@ -561,6 +561,7 @@ impl AggregateUDFImpl for ForeignAggregateUDF {
 pub enum FFI_AggregateOrderSensitivity {
     Insensitive,
     HardRequirement,
+    SoftRequirement,

Review Comment:
   Technically speaking this is an FFI API change -- I am not sure what 
implication that has (note this would not be released until DataFusion 50 
anyways).
   
   cc @timsaucer  -- I wonder if we should gather up the FFI breaking changes 
into their own PR / more carefully schedule such breakages



##########
datafusion/functions-aggregate-common/src/order.rs:
##########
@@ -25,6 +25,10 @@ pub enum AggregateOrderSensitivity {
     /// The aggregator can not produce a correct result unless its ordering
     /// requirement is satisfied.
     HardRequirement,
+    /// Indicates that the aggregate expression strongly prefers the input to 
be ordered.
+    /// The aggregator can produce its result correctly regardless of the 
input ordering,
+    /// This is a similar to, but stronger than, `Beneficial`.

Review Comment:
   I think it would help to explain why the aggregate expression might prefer 
the input to be ordered. 
   
   Maybe we can incorporate the comments from @ozankabak here: 
https://github.com/apache/datafusion/pull/16625#issuecomment-3031665846
   
   Something like
   
   ```suggestion
       /// Indicates that the aggregate expression strongly prefers the input 
to be ordered.
       /// Similarly to [`Self::HardRequirement`], when possible DataFusion 
will insert 
       /// a `SortExec`, to reorder the input to match the SoftRequirement. 
However, 
       /// when such a sort cannot be inserted, no error will be thrown, unlike 
with 
       /// with [`Self::HardRequirement`]
       /// 
       /// The aggregator can produce its result correctly regardless of the 
input ordering,
       /// This is a similar to, but stronger than, `Beneficial`.
   ```



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