Add interval mul/div support for Float64#10409
Open
peterxcli wants to merge 1 commit into
Open
Conversation
Member
Author
Contributor
|
Thanks - I'll try and find time for this but I need to focus on performance and bugfixes first |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
#10336 added interval multiplication by
Int64. Floating-point multiplication and division require preserving calendar components rather than flattening the entire interval into a duration.This PR adapts DuckDB's
INTERVAL * DOUBLEbehavior. Whole months and days remain calendar components, while fractional months cascade to days using 30 days per month and fractional days cascade to nanoseconds using 24 hours per day.DuckDB explicitly attributes its implementation to PostgreSQL's
interval_mul:What changes are included in this PR?
Add checked arithmetic for:
Interval(YearMonth | DayTime | MonthDayNano) * Float64Float64 * Interval(YearMonth | DayTime | MonthDayNano)Interval(YearMonth | DayTime | MonthDayNano) / Float64All results use
Interval(MonthDayNano).YearMonthandDayTimeinputs are widened first so fractional months and sub-millisecond results are not truncated.Are these changes tested?
Float64factors, preserving exact nanosecondsInterval / Int64,Float64 / Interval, wrapping multiplication, and additional numeric factor types are not added.Are these changes tested?
Yes. Tests cover all three interval units, both multiplication operand orders, division, array and scalar operands, null propagation, component cascading, integral-factor precision, negative values, ties-to-even rounding, non-finite values, division by zero, and overflow.
cargo fmt --all -- --check cargo test -p arrow-arith cargo clippy -p arrow-arith --all-targets --all-features -- -D warningsAre there any user-facing changes?
Yes. The checked
mulanddivkernels now accept the combinations listed above. Floating-point interval arithmetic always returnsInterval(MonthDayNano).There are no public API signature changes.