Refactor to_local_time() signature away from user_defined
#18707
Conversation
github-actions
bot
added
sqllogictest
| signature: Signature::coercible( | ||
| vec![Coercion::new_exact(TypeSignatureClass::Timestamp)], | ||
| Volatility::Immutable, | ||
| ), |
There was a problem hiding this comment.
Signature change here
| } | ||
| } | ||
|
|
||
| fn to_local_time(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> { |
There was a problem hiding this comment.
I refactored to_local_time to move it outside of ToLocalTimeFunc struct since it didn't really need the self
|
|
||
| fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> { | ||
| match &arg_types[0] { | ||
| DataType::Null => Ok(Timestamp(Nanosecond, None)), |
There was a problem hiding this comment.
Matching the old behaviour wrt nulls here
| Ok(ColumnarValue::Array(Arc::new(builder.finish()))) | ||
| } | ||
|
|
||
| fn to_local_time(time_value: &ColumnarValue) -> Result<ColumnarValue> { |
There was a problem hiding this comment.
Refactored to_local_time function here now; I also flattened the function since it was quite nested before
|
|
||
| # invalid argument data type | ||
| statement error The to_local_time function can only accept Timestamp as the arg got Utf8 | ||
| statement error DataFusion error: Error during planning: Internal error: Expect TypeSignatureClass::Timestamp but received NativeType::String, DataType: Utf8. |
There was a problem hiding this comment.
The only actual change to this file (rest is whitespace changes).
Since we're using signature API now the default error changes; technically it is less friendly than what we had before, but it means it's centralized and we can work towards ensuring the message is better for the whole signature coercion API
1 participant
Which issue does this PR close?
Part of #12725
Rationale for this change
Prefer to avoid user_defined for consistency in function definitions.
What changes are included in this PR?
Refactor signature of
to_local_timeaway from user_defined.Also some other refactors for
to_local_time.Are these changes tested?
Existing tests.
Are there any user-facing changes?
No.