Double type argument for to_timestamp function #8159
Conversation
github-actions
bot
added
logical-expr
|
How can I resolve the one failed check |
One trick is to close/reopen the PR Anther is to merge up to latest master (which will retrigger the PR) |
There was a problem hiding this comment.
This might be affected by #8193
Thanks @spaydar
Please add tests for cast, ::timestamp as well like in https://github.com/apache/arrow-datafusion/pull/8193/files
all 3 functions should work the same
|
Marking as draft to signify this isn't waiting on feedback anymore. Please mark it as ready for review when it is |
| if expr_type == cast_type { | ||
| Ok(expr.clone()) | ||
| } else if can_cast_types(&expr_type, &cast_type) { | ||
| } else if can_cast_types(&expr_type, &cast_type) |
There was a problem hiding this comment.
I think we need to have a followup PR in arrow-rs, I'll do it
There was a problem hiding this comment.
Yes, if this can be pushed down to the arrow crate, the complexity in datafusion would be reduced. I wasn't sure if doing so was appropriate
There was a problem hiding this comment.
I just checked arrow-rs so such cast is supported https://github.com/apache/arrow-rs/blob/master/arrow-cast/src/cast.rs#L224
There was a problem hiding this comment.
can_cast_types was returning false in my testing yesterday for Float64 -> Timestamp(Nanosecond, None), seemingly because the line you linked has not been released yet. The type check was changed from is_integer to is_numeric only a few days ago, whereas the last arrow-rs release was 3 weeks ago.
Should we wait until the next arrow-rs release so I can leverage this change?
There was a problem hiding this comment.
I'm okay to let it go, because this is important piece. I'll create a followup issue to move to arrow-rs cast and small other refactoring. Thanks @spaydar for your work
| { | ||
| if let ScalarValue::Float64(Some(float_ts)) = scalar { | ||
| ScalarValue::Int64( | ||
| Some((float_ts * 1_000_000_000_f64).trunc() as i64), |
There was a problem hiding this comment.
I think here complexity can be reduced. too many conditions
There was a problem hiding this comment.
If we are allowing this type cast to be pushed down to the arrow crate, then I can add this logic to kernel::compute::cast_with_options or the appropriate fn
|
Filed #8370 I'll pick it up when arrow-rs has released |
* feat: test queries for to_timestamp(float) WIP * feat: Float64 input for to_timestamp * cargo fmt * clippy * docs: double input type for to_timestamp * feat: cast floats to timestamp * style: cargo fmt * fix: float64 cast for timestamp nanos only
3 participants
Which issue does this PR close?
Closes #7868.
Rationale for this change
Supports
doubletype argument forto_timestampfunction to align with Postgres.What changes are included in this PR?
This PR adds
doubletype argument for theto_timestampfunction, however this function still returns atimestamptype and not atimestamp with time zonetype as it does in Postgres because timezone support forto_timestamp*()functions in DataFusion is still an open issue.Are these changes tested?
Yes, there are
sqllogictests for this changeAre there any user-facing changes?
Yes, an additional type is added to the
to_timestampfunction signature. This change has been added to the User Guide documentation.