Skip to content
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.

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

Fix overflow for min calculation in strings::from_timestamps #9793

Merged
merged 1 commit into from
Dec 1, 2021

Conversation

revans2
Copy link
Contributor

@revans2 revans2 commented Nov 29, 2021

This fixes #9790

When converting a timestamp to a String it is possible for the %M min calculation to overflow an int32_t part way through casting. This moves that result to be an int64_t which avoids the overflow issues.

@revans2 revans2 added bug Something isn't working 3 - Ready for Review Ready for review by team Spark Functionality that helps Spark RAPIDS strings strings issues (C++ and Python) non-breaking Non-breaking change labels Nov 29, 2021
@revans2 revans2 self-assigned this Nov 29, 2021
@revans2 revans2 requested a review from a team as a code owner November 29, 2021 20:05
@revans2 revans2 requested a review from bdice November 29, 2021 20:05
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Nov 29, 2021
@codecov
Copy link

codecov bot commented Nov 29, 2021

Codecov Report

Merging #9793 (725ad7b) into branch-22.02 (967a333) will decrease coverage by 0.00%.
The diff coverage is 12.35%.

❗ Current head 725ad7b differs from pull request most recent head 5664728. Consider uploading reports for the commit 5664728 to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.02    #9793      +/-   ##
================================================
- Coverage         10.49%   10.48%   -0.01%     
================================================
  Files               119      119              
  Lines             20305    20339      +34     
================================================
+ Hits               2130     2133       +3     
- Misses            18175    18206      +31     
Impacted Files Coverage Δ
python/cudf/cudf/__init__.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/column.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/string.py 0.00% <ø> (ø)
python/cudf/cudf/core/frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/index.py 0.00% <ø> (ø)
python/cudf/cudf/core/indexed_frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/multiindex.py 0.00% <0.00%> (ø)
python/cudf/cudf/utils/utils.py 0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/csv.py 96.00% <100.00%> (+0.10%) ⬆️
python/dask_cudf/dask_cudf/io/tests/test_csv.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1811b5...5664728. Read the comment docs.

@davidwendt
Copy link
Contributor

Please add a more detailed description to this PR since it will appear in the change log at release time.

@revans2 revans2 changed the title Fix overflow in strings::from_timestamps Fix overflow for min calculation in strings::from_timestamps Nov 30, 2021
@revans2
Copy link
Contributor Author

revans2 commented Nov 30, 2021

rerun tests

@@ -707,9 +707,9 @@ struct from_timestamp_base {
* scale( 61,60) -> 1
* @endcode
*/
__device__ int32_t scale_time(int64_t time, int64_t base) const
__device__ int64_t scale_time(int64_t time, int64_t base) const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This most certainly would have been avoided if this code used chrono types directly instead of trying circumvent the types by using integers.

Filed #9812

@revans2
Copy link
Contributor Author

revans2 commented Dec 1, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 1904d1a into rapidsai:branch-22.02 Dec 1, 2021
@revans2 revans2 deleted the fix_ts_to_string_mins branch December 2, 2021 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Spark Functionality that helps Spark RAPIDS strings strings issues (C++ and Python)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] strings::from_timestamp can overflow on valid timestamps
4 participants