Conversation
Weijun-H
force-pushed
the
fix-cast-lit-to-timestamp
branch
from
09bfc67 to
3d4db81
Compare
| } | ||
|
|
||
| #[test] | ||
| fn test_try_cast_literal_to_timestamp() { |
There was a problem hiding this comment.
please add more tests between timestamps
|
|
||
| /// Cast a timestamp value from one unit to another | ||
| fn cast_between_timestamp(from: DataType, to: DataType, value: i128) -> Option<i64> { | ||
| let seconds = match from { |
There was a problem hiding this comment.
I think it can be improved. if the cast Second to Second, then the code will do unneccessary mul by 1m and then div by 1m
| } | ||
|
|
||
| /// Cast a timestamp value from one unit to another | ||
| fn cast_between_timestamp(from: DataType, to: DataType, value: i128) -> Option<i64> { |
There was a problem hiding this comment.
I think this has problematic overflow behaviour, I wonder if we can determine the scale factor and multiply or divide by this instead? This is what arrow-cast does - https://github.com/apache/arrow-rs/blob/master/arrow-cast/src/cast.rs#L1671
Currently I think converting i64::MAX / 1_000 to milliseconds from seconds will silently overflow, when in reality it shouldn't
| ) | ||
| .unwrap() | ||
| .unwrap(); | ||
| assert_eq!(new_scalar, ScalarValue::TimestampMillisecond(None, None)); |
|
Benchmark runs are scheduled for baseline = 9464bf2 and contender = 1f8ede5. 1f8ede5 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
6 participants
Which issue does this PR close?
Closes #5507
Rationale for this change
When converting between different timestamps, reduce the precision by truncating excess digits.
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?