mbutrovich commented on code in PR #26686:
URL: https://github.com/apache/datafusion/pull/26686#discussion_r4367251070
##########
datafusion/spark/src/function/datetime/date_part.rs:
##########
@@ -81,10 +81,10 @@ impl ScalarUDFImpl for SparkDatePart {
internal_err!("Use return_field_from_args in this case instead.")
}
- fn return_field_from_args(&self, args: ReturnFieldArgs) ->
Result<FieldRef> {
- let nullable = args.arg_fields.iter().any(|f| f.is_nullable());
-
- Ok(Arc::new(Field::new(self.name(), DataType::Int32, nullable)))
+ fn return_field_from_args(&self, _args: ReturnFieldArgs) ->
Result<FieldRef> {
+ // Nullable like the integer parts of `date_part`, which this
simplifies
+ // to, because they can be NULL for a non-null input
+ Ok(Arc::new(Field::new(self.name(), DataType::Int32, true)))
}
Review Comment:
> So `return_type` could return `Ok(DataType::Int32)` and the override could
go.
Done in
[`date_part.rs`](https://github.com/apache/datafusion/blob/48ff4d8da0d3a990f6ef216e13ef4e1f5cbe758f/datafusion/spark/src/function/datetime/date_part.rs#L79-L84).
A [`DESCRIBE`
query](https://github.com/apache/datafusion/blob/48ff4d8da0d3a990f6ef216e13ef4e1f5cbe758f/datafusion/sqllogictest/test_files/spark/datetime/date_part.slt#L288-L293)
replaces the unit test.
> Another option is to delegate to
`datafusion_functions::datetime::date_part().return_field_from_args(args)`,
which keeps the two matching by construction.
The `nanosecond` and `epoch` mismatch is also on `main`
([Spark](https://github.com/apache/datafusion/blob/c8c046491ed44f0f969695f4aeade4e70af3317e/datafusion/spark/src/function/datetime/date_part.rs#L84-L87),
[`date_part`](https://github.com/apache/datafusion/blob/c8c046491ed44f0f969695f4aeade4e70af3317e/datafusion/functions/src/datetime/date_part.rs#L194-L198)).
Delegating would make the two agree, but neither would match Spark. Spark's
[`parseExtractField`](https://github.com/apache/spark/blob/cecbd5d846a2cdd891a4d9f0bda302b58b02092a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala#L4567-L4597)
rejects `NANOSECOND` and `EPOCH`, and its `SECOND` is
[`SecondWithFraction`](https://github.com/apache/spark/blob/cecbd5d846a2cdd891a4d9f0bda302b58b02092a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala#L665-L669),
a `DECIMAL(8, 6)` that keeps the fraction. Could we fix that in a separate PR,
b
y rejecting the parts Spark rejects and returning Spark's type for `SECOND`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
