Uh oh!
There was an error while loading. Please reload this page.
Double type argument for to_timestamp function - #8159
Conversation
spaydar
commented
Nov 14, 2023
How can I resolve the one failed check |
alamb
commented
Nov 16, 2023
One trick is to close/reopen the PR Anther is to merge up to latest master (which will retrigger the PR) |
comphead
left a comment
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
spaydar
commented
Nov 27, 2023
alamb
commented
Nov 27, 2023
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_numericonly 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
comphead
commented
Nov 30, 2023
Filed #8370 I'll pick it up when arrow-rs has released |
alamb
commented
Nov 30, 2023
* 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
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.