refactor!: consistent null handling in coercible signatures #15404
Conversation
github-actions
bot
added
logical-expr
| repeat Int64 2 IN 3 | ||
| repeat LargeUtf8 1 IN 1 | ||
| repeat LargeUtf8 1 OUT 1 | ||
| repeat Null 1 IN 3 | ||
| repeat Null 1 OUT 3 |
There was a problem hiding this comment.
I'm not sure why this was changed, as there seems to already be a test case for SELECT repeat(NULL, 4) in the original version, so the information schema shouldn't need to be modified?
There was a problem hiding this comment.
I think this makes sense? Since it was previously accepted by the matches_native_type without needing to be listed as one of the possible signatures. Now we have added to the possible cartesian products per here.
| repeat Int64 2 IN 3 | ||
| repeat LargeUtf8 1 IN 1 | ||
| repeat LargeUtf8 1 OUT 1 | ||
| repeat Null 1 IN 3 | ||
| repeat Null 1 OUT 3 |
There was a problem hiding this comment.
I think this makes sense? Since it was previously accepted by the matches_native_type without needing to be listed as one of the possible signatures. Now we have added to the possible cartesian products per here.
alan910127
force-pushed
the
refactor/coercible-signature-consistent-null-handling
branch
from
e2dba0b to
680383b
Compare
There was a problem hiding this comment.
Thanks @alan910127 and @wiedld
I am worried about the potential subtle semantic issues in downstream projects that would result from this change (see comment). I worry about such changes especially when the benefits of the change are nicer internal datafusion code, but no benefit to downstream crates, although they will pay the cost
I know @shehabgamin was thinking about coercion recently -- maybe he would have some time to review this PR / comment about its potential implications and how it would fit into a broader story about coercion improvements
| @@ -263,10 +263,6 @@ impl TypeSignatureClass { | |||
| self: &TypeSignatureClass, | |||
| logical_type: &NativeType, | |||
| ) -> bool { | |||
| if logical_type == &NativeType::Null { | |||
There was a problem hiding this comment.
The PR description says
Are there any user-facing changes?
No.
However, doesn't this change means that existing TypeSignature::Coercable that handle Nulls will need to add Coercion::new_implicit to retain the same behavior?
There was a problem hiding this comment.
Oh, that's right, I forgot about that. PR description updated.
alan910127
changed the title
There was a problem hiding this comment.
Thank you @alan910127 @wiedld and @shehabgamin
Can you please add a brief note in the upgrade guide for 47.0.0 (eta next week) in https://github.com/apache/datafusion/blob/main/docs/source/library-user-guide/upgrading.md
About what a user will need to do when upgrading to 47.0.0? Specifically,
- What kind of UDFs will need to be updated
- What changes need to be done? (you could use examples from this PR)
github-actions
bot
added
the
documentation
|
Hi @alamb, I’ve updated |
|
Thanks @alan910127 -- I'll check it out in a few days |
Which issue does this PR close?
Nulltype inTypeSignatureClassmore consistently #15013.Rationale for this change
Currently, there is a special case for null handling in
TypeSignatureClass::matches_native_type, which causes inconsistencies and issues betweenTypeSignatureClass::matches_native_typeandTypeSignatureClass::default_casted_type(it will hit the else match arm which returns an error) whenselfis not aTypeSignatureClass::Native.What changes are included in this PR?
TypeSignatureClass::matches_native_typeTypeSignatureClass::Native(logical_null())where the argument is nullable and it was not originally supplied.Are these changes tested?
These changes should be covered in existing test.
Are there any user-facing changes?
Yes, if the user rely on the special null case handling behavior of 'TypeSignatureClass::Native
, they will need to transform the usage toCoercible::new_implicitand add aTypeSignatureClass::Native(logical_null())`.