feat: add bounds for unary math scalar functions #11584
Conversation
github-actions
bot
added
logical-expr
| pub(super) fn unbounded(_input: &[&Interval]) -> crate::Result<Interval> { | ||
| // We cannot assume the input datatype is the same of output type. | ||
| Interval::make_unbounded(&DataType::Null) | ||
| } |
There was a problem hiding this comment.
Is it better here returning not_impl_err? The current version may cause type mismatch errors and debugging would require more time.
There was a problem hiding this comment.
I think that would cause an error when trying to use these functions? The update macro is going to call whatever is passed as $EVALUATE_BOUNDS. Right now this implementation matches the default of the ScalarUDFImpl trait.
I think an alternative is to make something like Interval::make_symmetric_inf. I'm using unbounded here somewhat loosely, but probably in most of these cases it should be (-inf, inf) which could be given a type.
There was a problem hiding this comment.
Went ahead and did that here: cf2b023
| @@ -332,6 +332,46 @@ impl Interval { | |||
| Ok(Self::new(unbounded_endpoint.clone(), unbounded_endpoint)) | |||
| } | |||
|
|
|||
| /// Creates an interval between -∞ to ∞. | |||
| pub fn make_infinity_interval(data_type: &DataType) -> Result<Self> { | |||
There was a problem hiding this comment.
We use ScalarValue::Float(None) for infinite bounds by convention. Even if you give Float(Some(f::INF)) or Float(Some(f::NaN)), they are converted to Float(None)'s during interval creation. We had given such a decision to have unique representation of unboundedness. You can check the details here:
I recommend here to use make_unbounded API with floating types to not break this convention.
| } | ||
|
|
||
| /// Create an interval from 0 to infinity. | ||
| pub fn make_non_negative_infinity_interval(data_type: &DataType) -> Result<Self> { |
There was a problem hiding this comment.
Same suggestion applies here as well
|
Thanks @tshauck. This is a nice step towards having comprehensive interval analysis. I have left a small suggestion. Once that is addressed, the PR will be good to go 🚀 |
|
Apart from @berkaysynnada's outstanding suggestions the only thing I see is that we would need to use two different π values, depending on whether it is in the lower bound or the upper bound of the resulting interval. If π will be in the upper bound, we should use the smallest floating point number that is greater than the mathematical (precise) value of π. Let's call this Since π is not exactly representable, the Apart from this detail that is addressable by a small fix, this seems to be a great PR. Thank you. |
|
Thanks to both of you for the thoughtful feedback. @berkaysynnada, I updated infinity to use unbounded in 231a717. @ozankabak, for handling π, do you mean |
|
IIRC nextafter is the IEEE standard name for this function, and Rust's next_up does something similar (in the upward direction). If it is in nightly, we probably should just create constants with these values explicitly given for each float type. |
|
Thanks @ozankabak, would you please have a look at the latest changes and lmk what you think? |
|
I checked the new version for both unbounded creation and rounded PI usage. Everything looks good and seems ready to merge. Thanks @tshauck! |
tshauck
force-pushed
the
add-unary-udf-bounds
branch
from
4337de8
to
a10a150
Compare
|
Thanks! -- it looks like |
|
I took a quick look and this looks good to me. Thank you @tshauck @berkaysynnada and @ozankabak 🚀 |
Which issue does this PR close?
Closes #11583 11583
Rationale for this change
Many math unary functions aren't unbounded, but because the macro doesn't include a way to modify this on a per function basis the trait default (of unbounded) is used.
What changes are included in this PR?
make_math_unary_udfmacro to take a bounds functionAre these changes tested?
Yes, see unittests.
Are there any user-facing changes?
No