Add is_integer API that can check for the validity of a string-to-integer conversion #7642

ttnghia · 2021-03-18T20:40:45Z

This PR addresses #5110, #7080, and rework #7094. It adds the function cudf::strings::is_integer that can check if strings can be correctly converted into integer values. Underflow and overflow are also taken into account.

Note that this cudf::strings::is_integer is different from the existing cudf::strings::string::is_integer, which only checks for pattern and does not care about under/overflow.

Examples:

s = { "eee", "-200", "-100", "127", "128", "1.5", NULL}

is_integer(s, INT8) = { 0, 0, 1, 1, 0, 0, NULL}
is_integer(s, INT32) = { 0, 1, 1, 1, 1, 0, NULL}

…dity of the conversion process

davidwendt · 2021-03-18T21:17:14Z

I don't agree with this change. We considered this in the initial design.
Invalid data checks should not be added to the conversion routines. Do not add this extra kernel call to the conversion process.
@jrhemstad @harrism @kkraus14

davidwendt

Do not add this extra kernel call to the conversion process.
Also, I would consider just adding a new is_integer API that takes a data_type rather change the current one.

ttnghia · 2021-03-18T21:29:52Z

Do not add this extra kernel call to the conversion process.
Also, I would consider just adding a new is_integer API that takes a data_type rather change the current one.

If not modifying the existing to_integer API then do you have any idea that we can have a new API for doing the job of my modified to_integer version? It would be great if we have it.

davidwendt · 2021-03-18T22:10:42Z

Do not add this extra kernel call to the conversion process.
Also, I would consider just adding a new is_integer API that takes a data_type rather change the current one.

If not modifying the existing to_integer API then do you have any idea that we can have a new API for doing the job of my modified to_integer version? It would be great if we have it.

You should be able to use a combination of is_integer and to_integer to achieve your result, right?
But this function should not be part of libcudf.

ttnghia · 2021-03-18T23:48:22Z

You should be able to use a combination of is_integer and to_integer to achieve your result, right?
But this function should not be part of libcudf.

Thanks David. I just checked with the Spark team and was verified that this can be done (in a less efficient way) in Spark, so I've reversed back my changes in the to_integer function.

codecov · 2021-03-19T18:43:12Z

Codecov Report

Merging #7642 (01aec79) into branch-0.19 (7871e7a) will increase coverage by 0.23%.
The diff coverage is n/a.

❗ Current head 01aec79 differs from pull request most recent head f2daef9. Consider uploading reports for the commit f2daef9 to get more accurate results

@@               Coverage Diff               @@
##           branch-0.19    #7642      +/-   ##
===============================================
+ Coverage        81.86%   82.10%   +0.23%     
===============================================
  Files              101      101              
  Lines            16884    17054     +170     
===============================================
+ Hits             13822    14002     +180     
+ Misses            3062     3052      -10

Impacted Files	Coverage Δ
python/cudf/cudf/core/column/categorical.py	`91.62% <ø> (+0.23%)`	⬆️
python/cudf/cudf/core/column/column.py	`87.77% <ø> (+0.01%)`	⬆️
python/cudf/cudf/core/column/datetime.py	`89.09% <ø> (ø)`
python/cudf/cudf/core/column/decimal.py	`92.75% <ø> (-2.12%)`	⬇️
python/cudf/cudf/core/column/lists.py	`92.17% <ø> (+0.77%)`	⬆️
python/cudf/cudf/core/column/numerical.py	`94.83% <ø> (-0.20%)`	⬇️
python/cudf/cudf/core/column/string.py	`86.58% <ø> (+0.08%)`	⬆️
python/cudf/cudf/core/column/timedelta.py	`88.23% <ø> (ø)`
python/cudf/cudf/core/column_accessor.py	`95.87% <ø> (+0.55%)`	⬆️
python/cudf/cudf/core/dataframe.py	`90.78% <ø> (+0.31%)`	⬆️
... and 27 more

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 30e493c...f2daef9. Read the comment docs.

cpp/tests/strings/integers_tests.cu

cpp/src/strings/convert/convert_integers.cu

…losing braces

cpp/tests/strings/integers_tests.cu

Co-authored-by: David <45795991+davidwendt@users.noreply.github.com>

cpp/src/strings/convert/convert_integers.cu

jrhemstad · 2021-03-23T18:44:29Z

cpp/src/strings/convert/convert_integers.cu

+  thrust::transform(rmm::exec_policy(stream),
+                    thrust::make_counting_iterator<size_type>(0),
+                    thrust::make_counting_iterator<size_type>(strings.size()),
+                    d_results,
+                    [d_column] __device__(size_type idx) {
+                      if (d_column.is_null(idx)) return false;
+                      return string::is_integer(d_column.element<string_view>(idx));
+                    });


Suggested change

thrust::transform(rmm::exec_policy(stream),

thrust::make_counting_iterator<size_type>(0),

thrust::make_counting_iterator<size_type>(strings.size()),

d_results,

[d_column] __device__(size_type idx) {

if (d_column.is_null(idx)) return false;

return string::is_integer(d_column.element<string_view>(idx));

});

thrust::transform(rmm::exec_policy(stream),

strings_column.pair_begin(),

strings_column.pair_end(),

d_results,

[d_column] __device__(auto p) {

return p.second ? is_integer(p.first) : false;

});

This becomes even nicer when we replace pair_iterator with optional_iterator #6470

The reason I don't like this is because the pair iterator requires the has_nulls at compile time and thus this doubles the compile time. This looks nice and all but I'd rather we waited until the pair iterator was changed to make it a runtime parameter instead.

What is blocking that change?

@davidwendt please request changes when something is a problem otherwise the problem will get merged.

The main concern here is something like this:

if (strings.has_nulls()) { thrust::transform( rmm::exec_policy(stream), d_column->pair_begin<string_view, true>(), d_column->pair_end<string_view, true>(), d_results, [] __device__(auto const& p) { return p.second ? string::is_integer(p.first) : false; }); } else { thrust::transform( rmm::exec_policy(stream), d_column->pair_begin<string_view, false>(), d_column->pair_end<string_view, false>(), d_results, [] __device__(auto const& p) { return p.second ? string::is_integer(p.first) : false; }); }

So we will have 2 calls to thrust::transform instead of just one. I can verify that this makes the compile time increases by around 1 second---not a problem at all.

Before:

6:07 PM Build finished in 25 sec, 27 ms

After:

6:08 PM Build finished in 26 sec, 241 ms

harrism

Requesting changes until discussion about @davidwendt 's concerns on compile time is resolved.

jrhemstad · 2021-03-23T23:44:40Z

Requesting changes until discussion about @davidwendt 's concerns on compile time is resolved.

I don't think its worth worrying about right now. This file is not a top offender for compile time.

ttnghia · 2021-03-24T00:10:47Z

Requesting changes until discussion about @davidwendt 's concerns on compile time is resolved.

I don't think its worth worrying about right now. This file is not a top offender for compile time.

Compile time is affected by 1 second slower, so we don't need to worry about it yet:

Before:

6:07 PM	Build finished in 25 sec, 27 ms

After:


6:08 PM	Build finished in 26 sec, 241 ms

harrism · 2021-03-24T09:36:02Z

@gpucibot merge

ttnghia added 4 commits March 18, 2021 12:23

Finish is_integer tests with bound check

9a2229b

Implement is_integer with checking for both underflow and overflow

5854ebb

Re-implement to_integer which will re-generate bitmask using the vali…

34944f7

…dity of the conversion process

Rewrite tests for is_integer and to_integer with bound check

a53ecbe

ttnghia added feature request New feature or request 3 - Ready for Review Ready for review by team tests Unit testing for project libcudf Affects libcudf (C++/CUDA) code. strings strings issues (C++ and Python) non-breaking Non-breaking change labels Mar 18, 2021

ttnghia requested a review from a team as a code owner March 18, 2021 20:40

ttnghia requested review from trxcllnt and jrhemstad March 18, 2021 20:40

This was linked to issues Mar 18, 2021

[BUG] Cast from string to long should return null if string is not a valid long #5110

Closed

[FEA] add something like an is_valid_integer function #7080

Closed

ttnghia mentioned this pull request Mar 18, 2021

add is_valid_integer format check API #7094

Closed

ttnghia added 2 commits March 18, 2021 15:08

Add test for is_integer in case the input strings column has null

00ccbe4

Update doc

5e9293c

ttnghia changed the title ~~Casting from string to integer returns null if the string is not a valid integer~~ Add bounds check for is_integer and rework to_integer for string to integer conversion Mar 18, 2021

davidwendt requested changes Mar 18, 2021

View reviewed changes

ttnghia added 2 commits March 18, 2021 17:27

Reverse code for doing null check while calling to_integer

26c4772

Reverse tests for doing null check while calling to_integer

69ca9e5

ttnghia changed the title ~~Add bounds check for is_integer and rework to_integer for string to integer conversion~~ Add bounds check for is_integer Mar 18, 2021

Some small cleanups

239df00

ttnghia added 3 commits March 19, 2021 09:34

Rewrite tests for is_integer and to_integer

551f68d

Remove unused header

5502270

Fix doc

ef19286

ttnghia removed a link to an issue Mar 19, 2021

[BUG] Cast from string to long should return null if string is not a valid long #5110

Closed

davidwendt requested changes Mar 22, 2021

View reviewed changes

ttnghia added 3 commits March 22, 2021 11:44

Remove printing option from CUDF_TEST_EXPECT_COLUMNS_EQUAL

ae80834

Prevent ClangFormat from automatically reorganizing the namespaces' c…

4bd7954

…losing braces

Move the detail APIs out of the anonymous namespace

724d8e5

davidwendt reviewed Mar 22, 2021

View reviewed changes

cpp/tests/strings/integers_tests.cu Outdated Show resolved Hide resolved

Update cpp/tests/strings/integers_tests.cu

2fcd5a7

Co-authored-by: David <45795991+davidwendt@users.noreply.github.com>

davidwendt approved these changes Mar 22, 2021

View reviewed changes

jrhemstad reviewed Mar 23, 2021

View reviewed changes

cpp/src/strings/convert/convert_integers.cu Outdated Show resolved Hide resolved

jrhemstad reviewed Mar 23, 2021

View reviewed changes

ttnghia added 2 commits March 23, 2021 13:22

Merge branch 'branch-0.19' into convert_integer_bound_check

8d5895e

Change from using thrust::counting_iterator to pair iterator

f2daef9

jrhemstad approved these changes Mar 23, 2021

View reviewed changes

harrism requested changes Mar 23, 2021

View reviewed changes

harrism approved these changes Mar 24, 2021

View reviewed changes

rapids-bot bot merged commit de55832 into rapidsai:branch-0.19 Mar 24, 2021

sameerz mentioned this pull request Mar 30, 2021

[BUG] Cast from string to long should return null if string is not a valid long #5110

Closed

ttnghia self-assigned this Apr 25, 2021

ttnghia deleted the convert_integer_bound_check branch May 3, 2021 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add is_integer API that can check for the validity of a string-to-integer conversion #7642

Add is_integer API that can check for the validity of a string-to-integer conversion #7642

ttnghia commented Mar 18, 2021 •

edited

Loading

davidwendt commented Mar 18, 2021

davidwendt left a comment

ttnghia commented Mar 18, 2021

davidwendt commented Mar 18, 2021

ttnghia commented Mar 18, 2021 •

edited

Loading

codecov bot commented Mar 19, 2021 •

edited

Loading

jrhemstad Mar 23, 2021

jrhemstad Mar 23, 2021

davidwendt Mar 23, 2021

harrism Mar 23, 2021

harrism Mar 23, 2021

ttnghia Mar 24, 2021 •

edited

Loading

ttnghia Mar 24, 2021

harrism left a comment

jrhemstad commented Mar 23, 2021

ttnghia commented Mar 24, 2021

harrism commented Mar 24, 2021

Add is_integer API that can check for the validity of a string-to-integer conversion #7642

Add is_integer API that can check for the validity of a string-to-integer conversion #7642

Conversation

ttnghia commented Mar 18, 2021 • edited Loading

davidwendt commented Mar 18, 2021

davidwendt left a comment

Choose a reason for hiding this comment

ttnghia commented Mar 18, 2021

davidwendt commented Mar 18, 2021

ttnghia commented Mar 18, 2021 • edited Loading

codecov bot commented Mar 19, 2021 • edited Loading

Codecov Report

jrhemstad Mar 23, 2021

Choose a reason for hiding this comment

jrhemstad Mar 23, 2021

Choose a reason for hiding this comment

davidwendt Mar 23, 2021

Choose a reason for hiding this comment

harrism Mar 23, 2021

Choose a reason for hiding this comment

harrism Mar 23, 2021

Choose a reason for hiding this comment

ttnghia Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

ttnghia Mar 24, 2021

Choose a reason for hiding this comment

harrism left a comment

Choose a reason for hiding this comment

jrhemstad commented Mar 23, 2021

ttnghia commented Mar 24, 2021

harrism commented Mar 24, 2021

ttnghia commented Mar 18, 2021 •

edited

Loading

ttnghia commented Mar 18, 2021 •

edited

Loading

codecov bot commented Mar 19, 2021 •

edited

Loading

ttnghia Mar 24, 2021 •

edited

Loading