alamb commented on code in PR #6654:
URL: https://github.com/apache/arrow-datafusion/pull/6654#discussion_r1231005390
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -884,8 +885,13 @@ impl BuiltinScalarFunction {
],
self.volatility(),
),
- BuiltinScalarFunction::DateTrunc => Signature::exact(
- vec![Utf8, Timestamp(Nanosecond, None)],
+ BuiltinScalarFunction::DateTrunc => Signature::one_of(
Review Comment:
this clause seems unreachable given the change above to match
`BuiltinScalarFunction::DateTrunc | BuiltinScalarFunction::DateBin => {`
I would personally suggest keeping this change and removing the
`BuiltinScalarFunction::DateTrunc | BuiltinScalarFunction::DateBin => {` change
as it make the code more explicy
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -282,54 +341,64 @@ pub fn date_trunc(args: &[ColumnarValue]) ->
Result<ColumnarValue> {
Ok(match array {
ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(v, tz_opt)) => {
- let nano = (f)(*v)?;
- match granularity.as_str() {
- "minute" => {
- // trunc to minute
- let second = ScalarValue::TimestampNanosecond(
- Some(nano.unwrap() / 1_000_000_000 * 1_000_000_000),
- tz_opt.clone(),
- );
- ColumnarValue::Scalar(second)
+ _date_trunc(TimeUnit::Nanosecond, v, granularity.as_str(), f,
tz_opt)?
+ }
+ ColumnarValue::Scalar(ScalarValue::TimestampMicrosecond(v, tz_opt)) =>
{
+ _date_trunc(TimeUnit::Microsecond, v, granularity.as_str(), f,
tz_opt)?
+ }
+ ColumnarValue::Scalar(ScalarValue::TimestampMillisecond(v, tz_opt)) =>
{
+ _date_trunc(TimeUnit::Millisecond, v, granularity.as_str(), f,
tz_opt)?
+ }
+ ColumnarValue::Scalar(ScalarValue::TimestampSecond(v, tz_opt)) => {
+ _date_trunc(TimeUnit::Second, v, granularity.as_str(), f, tz_opt)?
+ }
+ ColumnarValue::Array(array) => {
+ let array_type = array.data_type();
Review Comment:
I must be missing something here -- it seems like `granularity` is not used
for the array variants
So like `date_trunc(x, 'minutes')` will return the same value as
`date_trunc(x, 'seconds')` which doesn't seem right
Maybe that indicates missing test coverage 🤔
--
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]
