Cleanup common parsing code in JSON, CSV reader #12022

karthikeyann · 2022-10-28T11:46:51Z

Description

This PR will cleanup nested json reader and csv reader's common parsing code.

Uses std::optional for indicating parsing failure in parse_numeric
Cleanup
- Removed decode_value as it only gives only specialization for timestamp and duration types, rest of types are passthrough.
- Unified decode_digit

Depends on #11898 and #12021

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

…es_stripping

vuule · 2022-11-09T21:53:46Z

cpp/src/io/utilities/parsing_utils.cuh

      }
      if (exponent != 0) { value *= exp10(double(exponent * exponent_sign)); }
    }
  }
-  if (!all_digits_valid) { return error_result; }
+  if (!all_digits_valid) { return {}; }


Let's be more explicit here. Current code could be mistaken for returning a zero.

Suggested change

if (!all_digits_valid) { return {}; }

if (!all_digits_valid) { return std::nullopt; }

Compiles locally, but CI build gives this error
04:37:13 $SRC_DIR/cpp/src/io/utilities/parsing_utils.cuh(240): error #20014-D: calling a __host__ function from a __host__ __device__ function is not allowed

The constructor from nullopt_t arg might be a host function. not constexpr.

@karthikeyann Hi,I also encountered this problem, did you solve this problem?

Yes. By callling default constructor of std::optional
if (!all_digits_valid) { return std::optional<T>{}; }

vuule · 2022-11-09T22:32:48Z

CC @galipremsagar for viz and Python perspective on this behavior change

vuule

Few small suggestions.
It's not obvious at first glance what the desired behavior is, especially since Pandas uses NaN (for it's own reasons). The main benefit of using null here is that we used to return zeroes for invalid integers :( Not a great stand-in for NaN.

cpp/src/io/utilities/parsing_utils.cuh

cpp/tests/io/csv_test.cpp

cpp/tests/io/json_test.cpp

python/cudf/cudf/tests/test_csv.py

…-parsing-error-null

karthikeyann · 2022-11-15T00:02:22Z

0e45d13
This commit reverts change of behaviour of nested json reader to return null element on parsing error of numeric types.
Breaking behaviour in read_csv, and read_json:

old behaviour: parsing error returns quiet NAN for that type as result for that row.
new behaviour: parsing error returns null for that row.

pushing this breaking change to next release.

vuule

Nice improvement.

vuule · 2022-11-15T00:37:07Z

cpp/src/io/utilities/parsing_utils.cuh

@@ -670,6 +597,7 @@ struct ConvertFunctor {
                                                      parse_options_view const& opts,
                                                      bool as_hex)
  {
+    // TODO what's invalid input


Can you please make these TODOs clearer if we're planning to merge them?
I looked into the code and don't see that invalid input is handled in a robust way in parse_decimal and to_timestamp. Is that what this TODO needs to remind us of?

Yes. this TODO is a reminder for future and also to remind reviewers to discuss and decide what is invalid input for this type.

karthikeyann · 2022-11-15T16:11:49Z

@gpucibot merge

karthikeyann · 2022-11-15T16:21:19Z

@gpucibot merge

…12272) This PR changes behaviour of nested json reader to return null element on parsing error of numeric types. Breaking behaviour in read_csv, and read_json: - old behaviour: parsing error returns quiet NAN for that type as result for that row. - new behaviour: parsing error returns null for that row. Moved commit 0e45d13 #12022 (comment) to 23.02 Authors: - Karthikeyan (https://github.com/karthikeyann) Approvers: - Lawrence Mitchell (https://github.com/wence-) - Vyas Ramasubramani (https://github.com/vyasr) - Vukasin Milovanovic (https://github.com/vuule) URL: #12272

karthikeyann added 19 commits September 30, 2022 16:03

trim quotes for non-string values in nested json parsing

4423d20

add pytest compare cudf, cudf_experimental on values with quotes

1d69a69

remove unnecessary quote removal in timestamp and duration decode_value

b680e3f

Merge branch 'branch-22.12' of github.com:rapidsai/cudf into fix-quot…

49bdbb6

…es_stripping

null check before removing quotes

46663cf

return std::optional from parse_numeric, for validating parsing

d2b76e3

add unit tests of corner cases

c953410

fix old json reader to treat "null" as string.

1646bcc

cleanup decode_digit

f0c468b

fix doc

cad413c

add TODO invalid input for decimal, duration, timestamp

bbb434a

fix bug in csv_reader_options construction in cython

0910f2e

add pytest for breaking behaviour in read_csv

31636ea

fix bug in csv_reader_options construction in cython

d2757dc

add pytest for breaking behaviour in read_csv

e26b3d8

Merge branch 'branch-22.12' of github.com:rapidsai/cudf into fix-quot…

27c8059

…es_stripping

parsing error in numeric types should make that element null

e119fac

revert parsing error changes

3f8eb52

unit tests for string quotes

e0f6e3d

karthikeyann added feature request New feature or request 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. 4 - Needs Review Waiting for reviewer to review or respond non-breaking Non-breaking change labels Oct 28, 2022

karthikeyann self-assigned this Oct 28, 2022

github-actions bot added the Python Affects Python cuDF API. label Oct 28, 2022

karthikeyann and others added 3 commits October 28, 2022 17:21

remove debug prints

1317be3

replace @param[in] with @param

ce863b3

nan fix moved to PR rapidsai#12022

d9fc33b

karthikeyann requested a review from vuule November 9, 2022 14:36

vuule reviewed Nov 9, 2022

View reviewed changes

cpp/src/io/utilities/parsing_utils.cuh Outdated Show resolved Hide resolved

cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved

cpp/tests/io/json_test.cpp Outdated Show resolved Hide resolved

cpp/tests/io/json_test.cpp Outdated Show resolved Hide resolved

galipremsagar reviewed Nov 9, 2022

View reviewed changes

python/cudf/cudf/tests/test_csv.py Outdated Show resolved Hide resolved

karthikeyann added 4 commits November 15, 2022 02:58

address review comments

d7fb88c

Merge branch 'branch-22.12' of github.com:rapidsai/cudf into fea-json…

6fbd8ac

…-parsing-error-null

replace std::nullopt

bea7fc3

remove breaking behavior on nulls

0e45d13

github-actions bot removed the Python Affects Python cuDF API. label Nov 14, 2022

karthikeyann changed the title ~~Null element for parsing error in numeric types in JSON, CSV reader~~ Cleanup common parsing code in JSON, CSV reader Nov 15, 2022

karthikeyann requested review from galipremsagar and vuule November 15, 2022 00:03

galipremsagar approved these changes Nov 15, 2022

View reviewed changes

vuule approved these changes Nov 15, 2022

View reviewed changes

karthikeyann added non-breaking Non-breaking change and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer 4 - Needs Review Waiting for reviewer to review or respond breaking Breaking change labels Nov 15, 2022

TODO clarify

df6844c

rapids-bot bot merged commit fd488cd into rapidsai:branch-22.12 Nov 15, 2022

karthikeyann mentioned this pull request Nov 30, 2022

Null element for parsing error in numeric types in JSON, CSV reader #12272

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup common parsing code in JSON, CSV reader #12022

Cleanup common parsing code in JSON, CSV reader #12022

karthikeyann commented Oct 28, 2022 •

edited

Loading

vuule Nov 9, 2022

karthikeyann Nov 14, 2022

newjavaer Jan 29, 2023

karthikeyann Jan 29, 2023

vuule commented Nov 9, 2022

vuule left a comment

karthikeyann commented Nov 15, 2022

vuule left a comment

vuule Nov 15, 2022

karthikeyann Nov 15, 2022

karthikeyann commented Nov 15, 2022

karthikeyann commented Nov 15, 2022

	if (!all_digits_valid) { return {}; }
	if (!all_digits_valid) { return std::nullopt; }

Cleanup common parsing code in JSON, CSV reader #12022

Cleanup common parsing code in JSON, CSV reader #12022

Conversation

karthikeyann commented Oct 28, 2022 • edited Loading

Description

Checklist

vuule Nov 9, 2022

Choose a reason for hiding this comment

karthikeyann Nov 14, 2022

Choose a reason for hiding this comment

newjavaer Jan 29, 2023

Choose a reason for hiding this comment

karthikeyann Jan 29, 2023

Choose a reason for hiding this comment

vuule commented Nov 9, 2022

vuule left a comment

Choose a reason for hiding this comment

karthikeyann commented Nov 15, 2022

vuule left a comment

Choose a reason for hiding this comment

vuule Nov 15, 2022

Choose a reason for hiding this comment

karthikeyann Nov 15, 2022

Choose a reason for hiding this comment

karthikeyann commented Nov 15, 2022

karthikeyann commented Nov 15, 2022

karthikeyann commented Oct 28, 2022 •

edited

Loading