Skip to content

[C++] Division with negative duration types leads to NaN. #39156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
randolf-scholz opened this issue Dec 9, 2023 · 0 comments · Fixed by #39158
Closed

[C++] Division with negative duration types leads to NaN. #39156

randolf-scholz opened this issue Dec 9, 2023 · 0 comments · Fixed by #39158
Assignees
@js8544
Labels
Component: Python Type: bug
Milestone
15.0.0

Comments

@randolf-scholz
Copy link

randolf-scholz commented Dec 9, 2023 •
edited
Loading

Describe the bug, including details regarding any error messages, version, and platform.

import pyarrow as pa

pos_one_second = pa.scalar(1, type=pa.duration("us"))  # 1s
pos_two_second = pa.scalar(2, type=pa.duration("us"))  # 2s
neg_one_second = pa.scalar(-1, type=pa.duration("us"))  # -1s
neg_two_second = pa.scalar(-2, type=pa.duration("us"))  # -2s

print(pa.compute.divide(pos_one_second, pos_one_second))  # 1.0 ✔
print(pa.compute.divide(pos_two_second, pos_one_second))  # 2.0 ✔
print(pa.compute.divide(pos_one_second, pos_two_second))  # 0.5 ✔
print(pa.compute.divide(pos_two_second, pos_two_second))  # 1.0 ✔

print(pa.compute.divide(pos_one_second, pos_one_second))  # 1.0 ✔
print(pa.compute.divide(neg_one_second, pos_one_second))  # NAN ✘
print(pa.compute.divide(pos_one_second, neg_one_second))  # NAN ✘
print(pa.compute.divide(neg_one_second, neg_one_second))  # NAN ✘

print(pa.compute.divide(pos_one_second, pos_one_second))  # 1.0 ✔
print(pa.compute.divide(neg_two_second, pos_one_second))  # NAN ✘
print(pa.compute.divide(pos_one_second, neg_two_second))  # NAN ✘
print(pa.compute.divide(neg_two_second, neg_two_second))  # NAN ✘

Tested with 14.0.1, first noted here: pandas-dev/pandas#56409

Related:

#36789

Component(s)

Python
This was referenced Dec 9, 2023
Closed
Closed
@github-actions github-actions bot mentioned this issue Dec 9, 2023
Merged
@kou kou changed the title Division with negative duration types leads to NaN. [C++] Division with negative duration types leads to NaN. Dec 9, 2023
kou pushed a commit that referenced this issue Dec 10, 2023
@js8544
GH-39156: [C++][Compute] Fix negative duration division (#39158)
cca5eec 
### Rationale for this change

I forgot to cast durations to doubles in the current `division(duration, duration)` kernel. So they were essentially `reinterpret_cast`ed  to double. Because I only tested small positive ints but not large ints or negative ints, I missed this bug.

### What changes are included in this PR?

Add a `FloatingDivide` operator that casts ints to doubles and do floating division. Replace the `division(duration, duration)` with this op.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.

* Closes: #39156

Authored-by: Jin Shang <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
@kou kou added this to the 15.0.0 milestone Dec 10, 2023
mapleFU pushed a commit to mapleFU/arrow that referenced this issue Dec 13, 2023
@js8544 @mapleFU
apacheGH-39156: [C++][Compute] Fix negative duration division (apache…
7a1967a 
…#39158)

### Rationale for this change

I forgot to cast durations to doubles in the current `division(duration, duration)` kernel. So they were essentially `reinterpret_cast`ed  to double. Because I only tested small positive ints but not large ints or negative ints, I missed this bug.

### What changes are included in this PR?

Add a `FloatingDivide` operator that casts ints to doubles and do floating division. Replace the `division(duration, duration)` with this op.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.

* Closes: apache#39156

Authored-by: Jin Shang <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
clayburn pushed a commit to clayburn/arrow that referenced this issue Jan 23, 2024
@js8544 @clayburn
apacheGH-39156: [C++][Compute] Fix negative duration division (apache…
76956da 
…#39158)

### Rationale for this change

I forgot to cast durations to doubles in the current `division(duration, duration)` kernel. So they were essentially `reinterpret_cast`ed  to double. Because I only tested small positive ints but not large ints or negative ints, I missed this bug.

### What changes are included in this PR?

Add a `FloatingDivide` operator that casts ints to doubles and do floating division. Replace the `division(duration, duration)` with this op.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.

* Closes: apache#39156

Authored-by: Jin Shang <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
@js8544 @dgreiss
apacheGH-39156: [C++][Compute] Fix negative duration division (apache…
664b6a5 
…#39158)

### Rationale for this change

I forgot to cast durations to doubles in the current `division(duration, duration)` kernel. So they were essentially `reinterpret_cast`ed  to double. Because I only tested small positive ints but not large ints or negative ints, I missed this bug.

### What changes are included in this PR?

Add a `FloatingDivide` operator that casts ints to doubles and do floating division. Replace the `division(duration, duration)` with this op.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.

* Closes: apache#39156

Authored-by: Jin Shang <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@js8544 js8544

Labels
Component: Python Type: bug
Projects
None yet
Milestone
15.0.0
Development

Successfully merging a pull request may close this issue.

GH-39156: [C++][Compute] Fix negative duration division js8544/arrow
3 participants
@kou @js8544 @randolf-scholz