petern48 commented on code in PR #17491:
URL: https://github.com/apache/datafusion/pull/17491#discussion_r2337454988
##########
datafusion/functions/src/datetime/to_local_time.rs:
##########
@@ -119,6 +119,7 @@ impl ToLocalTimeFunc {
let arg_type = time_value.data_type();
match arg_type {
+ DataType::Null => Ok(ColumnarValue::Scalar(ScalarValue::Null)),
Review Comment:
It works either way. That null value is already casted to a timestamp type
in the current code. Running `select arrow_typeof(to_local_time(null));`
returns `Timestamp(Nanosecond, None)`.
> what needs to happen for existing tests to catch situations where type of
data returned from a function doesn't match the promised type?
The sqllogictests already account for this. In the test I created, the `P`
in `query P` enforces the timestamp type. For example, changing the `P` to `T`
(text) makes the test fail.
```sql
query P
select to_local_time(NULL);
```
I personally still like returning `ScalarValue::Null` here so we wouldn't
have to modify the code in multiple places in the event where we want to change
the datatype for some reason. @findepi WDYT?
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
