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


##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + 
Sync {
         Ok(ExprSimplifyResult::Original(args))
     }
 
-    /// Returns the [preimage] for this function and the specified scalar 
value, if any.
+    /// Returns a single contiguous preimage for this function and the 
specified
+    /// scalar expression, if any.
     ///
-    /// A preimage is a single contiguous [`Interval`] of values where the 
function
-    /// will always return `lit_value`
+    /// # Return Value

Review Comment:
   Moved discussion of return value to its own section and clearly separated 
the background from the API details



##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + 
Sync {
         Ok(ExprSimplifyResult::Original(args))
     }
 
-    /// Returns the [preimage] for this function and the specified scalar 
value, if any.
+    /// Returns a single contiguous preimage for this function and the 
specified
+    /// scalar expression, if any.
     ///
-    /// A preimage is a single contiguous [`Interval`] of values where the 
function
-    /// will always return `lit_value`
+    /// # Return Value
     ///
-    /// Implementations should return intervals with an inclusive lower bound 
and
-    /// exclusive upper bound.
+    /// Implementations should return a half-open interval: inclusive lower
+    /// bound and exclusive upper bound. Note that this is slightly different

Review Comment:
   one thing I think would make this API simpler would be to adjust the 
semantics so that the returned Interval used the same semantics as normal 
Intervals (entirely closed, so including the endpoints)
   
   Not sure what you think about this @sdf-jkl 



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

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to