Add is_integer API that can check for the validity of a string-to-integer conversion by ttnghia · Pull Request #7642 · NVIDIA/cudf

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.

…overflow

…API and fix null_count

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.

…losing braces

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

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

+  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 jrhemstad and trxcllnt 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 changed the title ~~Add bounds check for is_integer~~ Add is_integer API that can check for the validity of a string-to-integer conversion Mar 19, 2021

jrhemstad reviewed Mar 19, 2021

View reviewed changes

Comment thread cpp/tests/strings/integers_tests.cu Outdated

ttnghia added 7 commits March 19, 2021 09:00

Add back the original is_integer API that does not check for under/…

226e125

…overflow

Fix constness of mutable_view reference, add back the old is_integer …

e8b71b8

…API and fix null_count

Add comment to code

d6214e8

Just rename variable

41b5dab

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

Comment thread cpp/tests/strings/integers_tests.cu Outdated

Comment thread cpp/tests/strings/integers_tests.cu Outdated

Comment thread cpp/tests/strings/integers_tests.cu Outdated

Comment thread cpp/src/strings/convert/convert_integers.cu Outdated

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

Comment thread cpp/tests/strings/integers_tests.cu Outdated

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

Comment thread cpp/src/strings/convert/convert_integers.cu Outdated

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 NVIDIA: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

Uh oh!

Conversation

ttnghia commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidwendt commented Mar 18, 2021

Uh oh!

davidwendt left a comment

Choose a reason for hiding this comment

Uh oh!

ttnghia commented Mar 18, 2021

Uh oh!

davidwendt commented Mar 18, 2021

Uh oh!

ttnghia commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrhemstad Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

jrhemstad Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

davidwendt Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

harrism Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

harrism Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

ttnghia Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ttnghia Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

harrism left a comment

Choose a reason for hiding this comment

Uh oh!

jrhemstad commented Mar 23, 2021

Uh oh!

ttnghia commented Mar 24, 2021

Uh oh!

harrism commented Mar 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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