You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
GH-48003: [C++][Compute] Multiply and divide durations by float64 - #55460
There is no kernel for multiply(duration, float64) or divide(duration, float64), so these calls raise NotImplemented (see #48003).
What changes are included in this PR?
Kernels for multiply(duration, float64), multiply(float64, duration) and divide(duration, float64), registered for multiply, multiply_checked, divide and divide_checked. The result keeps the duration’s unit and is truncated toward zero, like numpy and divide(duration, int64). NaN, overflow and division by zero return an error. Only float64 is handled; float32 and float16 are not cast.
Are these changes tested?
Yes, a new test in scalar_temporal_test.cc (truncation, both argument orders, nulls, errors, all four time units) and a small Python test with the example from the issue.
Are there any user-facing changes?
Yes, these calls now return a duration instead of raising NotImplemented.
Was AI used for this PR?
In accordance with the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Human
AI
Reviewed before submission by:
Human
AI
Not reviewed
Code drafted by both human and AI, and reviewed by human.
Please do adversarial review with your agents. Have them look at other kernels. Once you've done that please review this yourself such that you know what each line does, then ping me.
Thanks for the guidance. I ran an adversarial review with two separate reviewers and compared the kernel against the existing ones (round, checked arithmetic, casts, divide(duration, int64)). From that, the kernel now uses the physical Int64Type like its neighbours, the test also covers the lower int64 bound, and the docs say the computation is done in float64. I switched to truncation toward zero, so Randolf’s example now gives exactly numpy’s output, and I added it as a Python test. I’ve gone through every line myself. Also, to correct my earlier comment: float32 and float16 are not cast in this PR.
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
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.
Rationale for this change
There is no kernel for
multiply(duration, float64)ordivide(duration, float64), so these calls raise NotImplemented (see #48003).What changes are included in this PR?
Kernels for
multiply(duration, float64),multiply(float64, duration)anddivide(duration, float64), registered formultiply,multiply_checked,divideanddivide_checked. The result keeps the duration’s unit and is truncated toward zero, like numpy anddivide(duration, int64). NaN, overflow and division by zero return an error. Only float64 is handled; float32 and float16 are not cast.Are these changes tested?
Yes, a new test in
scalar_temporal_test.cc(truncation, both argument orders, nulls, errors, all four time units) and a small Python test with the example from the issue.Are there any user-facing changes?
Yes, these calls now return a duration instead of raising NotImplemented.
Was AI used for this PR?
In accordance with the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Reviewed before submission by:
Code drafted by both human and AI, and reviewed by human.