Repository navigation
date_part returning wrong results due to overflows #14738
Description
Activity
It can also be replicated with intervals:
SELECT date_part('microseconds', interval '1 hour') -- returns -694967296, but the result should be 0
It seems like even without integer overflows, the overall logic is wrong:
SELECT date_part('seconds', interval '1 hour') -- returns 3600, but the result should be 0It seems like even without integer overflows, the overall logic is wrong:
SELECT date_part('seconds', interval '1 hour') -- returns 3600, but the result should be 0Hmm. I think this may be a separate issue as indeed it doesn't follow the typical pattern for that function where
SELECT date_part('seconds', interval '1 hour 5 second');
should return 5 (the seconds element in the interval, not the overall seconds the interval covers)
Appears to still be a problem
andrewlamb@Andrews-MacBook-Pro-3:~/Software/arrow-rs$ datafusion-cli DataFusion CLI v52.1.0 > SELECT date_part('microsecond', timestamp '1970-01-01T00:40:00' - timestamp '1970-01-01T00:00:00'); +------------------------------------------------------------------------------------------+ | date_part(Utf8("microsecond"),Utf8("1970-01-01T00:40:00") - Utf8("1970-01-01T00:00:00")) | +------------------------------------------------------------------------------------------+ | -1894967296 | +------------------------------------------------------------------------------------------+ 1 row(s) fetched. Elapsed 0.040 seconds.
I gave this a look and was able to fix it by casting duration array to nanosecond and then extracting components. However, it seemed like a fragile workaround so I was not sure of opening a PR.
Smth like this:
fn duration_part(array: &dyn Array, unit: IntervalUnit) -> Result<ArrayRef> { const NANOS_PER_MINUTE: i64 = 60 * 1_000_000_000; let divisor = match unit { Second => 1_000_000_000, Millisecond => 1_000_000, Microsecond => 1_000, Nanosecond => 1, }; let nanos_array = cast(array, &Duration(Nanosecond))?; let result = nanos_array.unary(|d| (d % NANOS_PER_MINUTE) / divisor); Ok(Arc::new(result)) }Should this be solved in https://github.com/apache/arrow-rs? Otherwise I can open a PR with the approach above
Should this be solved in https://github.com/apache/arrow-rs?
Yes I think that is probably the right approach
I'd like to work on this. I reproduced the issue on v55.0.0:
SELECT date_part( 'microsecond', timestamp '1970-01-01T00:40:00' - timestamp '1970-01-01T00:00:00' );
This currently panics in debug builds at
date_part.rs:426withattempt to multiply with overflow; release builds silently wrap and return an incorrect value.I also confirmed that the interval-semantics portion of this issue was fixed by #14817, so I intend to target only the remaining
Durationpath.My current plan is to perform the subsecond calculation using
i64internally while preserving the existingInt32return type. For values outside theInt32range, should this return an error or saturate/clamp?@alamb — you previously suggested the duration-part work might belong in arrow-rs. My proposed change is limited to fixing the existing
i32arithmetic in DataFusion'sdate_part.rsrather than introducing an upstream kernel. Would a local fix be appropriate here?If this approach sounds good, I'd be happy to take this issue.
Describe the bug
When playing with the date_part function, I see that there's ways of triggering int32 multiplication overflows that either panic on a debug build, or return the wrong number at runtime.
To Reproduce
Executing the following statement shows the behavior.
DataFusion fiddle link <- returns a wrong random number
Postgres fiddle link <- returns 0
Expected behavior
The date_part function should behave the same as Postgres
Additional context
Not 100% sure, but I would say that the changes introduced in #13466 look suspicious. There, the inner calculations are using int32 types, which are easy to overflow. Special mention to this line of code:
https://github.com/gabotechs/datafusion/blob/763bd681f09d58ce285ab3a677b81291c41adfce/datafusion/functions/src/datetime/date_part.rs#L290-L290