functions: Remove NullHandling from scalar funcs #14531
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
logical-expr
jkosh44
force-pushed
the
remove-null-handling
branch
from
3ce6857 to
383a618
Compare
|
Sorry for the spam of PRs related to this issue. It turns out, IMO, that the fix to the null input issue was improving the function signature and the |
3dfce7d added an enum to all scalar functions called `NullHandling`, with two variants: `PassThrough` and Propagate`. `PassThrough` would pass through null inputs to the function implementation. `Propagate` would cause the function to return null if any of the inputs were scalar and null, it would not do anything if an input was an array and null. Function implementors were responsible for handling null array inputs and making sure the behavior was consistent with the scalar null behavior caused by `NullHandling`. If the function signature correctly described the accepted types, then the null array input handling would also work for null scalar inputs. However, if the function signature was `VariadicAny`, then the null array input handling would not work for null scalar inputs. The reason is that when the signature is `VariadicAny`, null inputs are not properly typed (for example `ScalarValue::Null` instead of `ScalarValue::Int64(None)`. So it turns out that `NullHandling` was only useful for compensating for non-descriptive function signatures. Furthermore, many array functions use a signature of `VariadicAny` and reject invalid types within the function implementation. This does not work with `NullHandling::Propagate`, because any null input would skip the function implementation, which would skip the type validation. So, if a function wanted to use `NullHandling::Propagate`, then they would need to use a descriptive function signature. However, using a descriptive function signature removes the usefulness of `NullHandling::Propagate`. So as it turns out `NullHandling::Propagate` is never useful. For all the reasons stated above, this commit removes the `NullHandling` enum.
jkosh44
force-pushed
the
remove-null-handling
branch
from
383a618 to
2fd3514
Compare
|
@jayzhan211 I'm curious if you think this is a good idea or not. |
|
I also have a PoC for how we'd make the array signatures more expressive and more easily fix the null input errors: #14532 |
jayzhan211
approved these changes
Feb 7, 2025
|
Thanks @jayzhan211 and @jkosh44 |
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
3dfce7d added an enum to all scalar functions called
NullHandling, with two variants:PassThroughandPropagate.PassThroughwould pass through null inputs to the function implementation.Propagatewould cause the function to return null if any of the inputs were scalar and null, it would not do anything if an input was an array and null. Function implementors were responsible for handling null array inputs and making sure the behavior was consistent with the scalar null behavior caused byNullHandling.If the function signature correctly described the accepted types, then the null array input handling would also work for null scalar inputs. However, if the function signature was
VariadicAny, then the null array input handling would not work for null scalar inputs. The reason is that when the signature isVariadicAny, null inputs are not properly typed (for exampleScalarValue::Nullinstead ofScalarValue::Int64(None). So it turns out thatNullHandlingwas only useful for compensating for non-descriptive function signatures.Furthermore, many array functions use a signature of
VariadicAnyand reject invalid types within the function implementation. This does not work withNullHandling::Propagate, because any null input would skip the function implementation, which would skip the type validation. So, if a function wanted to useNullHandling::Propagate, then they would need to use a descriptive function signature. However, using a descriptive function signature removes the usefulness ofNullHandling::Propagate. So as it turns outNullHandling::Propagateis never useful.For all the reasons stated above, this commit removes the
NullHandlingenum.Which issue does this PR close?
Related to #10548
Are these changes tested?
Yes
Are there any user-facing changes?
No