[FIX] Fix incorrect nullCount in get_json_object

[trxcllnt]

Fixes an issue where get_json_object returns an incorrect null count on large inputs.

The source of the bug is that each thread was resetting out_valid_count to zero, then only the threads that execute in the final block contribute to the value of out_valid_count.

cc: @thomcom

[davidwendt]

Would it be possible to add a gtest that creates the issue?

[trxcllnt]

@davidwendt this is difficult to replicate in a gtest because it'll only manifest when there are more rows than threads (which also makes the bug GPU-dependent?). Do we have a way to generate large amounts of test data?

For reference, here's the valid counts before/after this change on a dataset with ~2M rows:

get_json_object valid count: 73280 <-- incorrect
get_json_object valid count: 1999999 <-- correct

[davidwendt]

this is difficult to replicate in a gtest because it'll only manifest when there are more rows than threads

Seems that it would occur when there are more rows than a block of threads if I'm reading the code right.
The block_size is hardcoded to 512

cudf/cpp/src/strings/json/json_path.cu

Line 987 in 1aa4ec8

constexpr int block_size = 512;

So perhaps a test with more than 512 rows would work?
Actually I guess it would have to be more rows than concurrent threads.

[davidwendt]

I no longer think a specific gtest is necessary here.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.10@4e45256). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files

@@               Coverage Diff               @@
##             branch-22.10   #11633   +/-   ##
===============================================
  Coverage                ?   86.41%           
===============================================
  Files                   ?      145           
  Lines                   ?    22993           
  Branches                ?        0           
===============================================
  Hits                    ?    19870           
  Misses                  ?     3123           
  Partials                ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ttnghia]

So this reset is no longer needed? I see that out_valid_count is then updated by atomicAdd, but what if out_valid_count was never been initialized before being passed into this function?

[davidwendt]

It is initialized here:

cudf/cpp/src/strings/json/json_path.cu

Line 1020 in 1aa4ec8

rmm::device_scalar<size_type> d_valid_count{0, stream};

I don't know why this if-statement was here. Maybe it was for some debug purpose.

[thomcom]

It is initialized here but I see your point. It can probably be safely initialized here, by threadIdx.x = 0, the thread that is responsible for loading the correct value.

[ttnghia]

If so then this looks somewhat unsafe IMO because this needs to rely on the caller to initialize the variable without any guarantee from anywhere.

If we decided to not have the initialization here, it is better to have a comment line in the function doxygen clearly stressing/clarifying that.

[thomcom]

I moved the initialization @ttnghia @davidwendt please re-review.

[trxcllnt]

If so then this looks somewhat unsafe IMO because this needs to rely on the caller to initialize the variable without any guarantee from anywhere.

Passing a pointer to an uninitialized device_scalar seems anti-RAII. If this were regular host code passing a pointer to a function and expecting that function to initialize it, without a very good reason (like that function has knowledge that the caller doesn't about how it should be initialized) I'd probably flag it as a code smell.

[thomcom]

@gpucibot merge