Support for string column to Parquet Variant infrastructure - #23614
Draft
abigalekim wants to merge 2 commits into
Draft
Support for string column to Parquet Variant infrastructure #23614abigalekim wants to merge 2 commits into
abigalekim wants to merge 2 commits into
Conversation
github-actions
Bot
added
libcudf
abigalekim
added
feature request
abigalekim
marked this pull request as draft
August 10, 2026 23:03
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe PR adds String-to-VARIANT encoding
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (1)
cpp/src/io/parquet/experimental/variant_encode.cu (1)
588-591: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCopy the input null mask once and reuse it.
cudf::detail::copy_bitmaskruns three times on the sameinput_null_mask: at line 590 for the value column, at line 606 for the struct column, and at line 454 insidemake_constant_metadata_column. Each call allocates a device buffer and launches a copy. Two of the three are avoidable.Copy the mask once before line 588 and construct the additional copies from that buffer, or pass the already-copied buffer into
make_constant_metadata_column.Also applies to: 604-607
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/io/parquet/experimental/variant_encode.cu` around lines 588 - 591, Update the surrounding encoding flow to copy input_null_mask only once, then reuse that device buffer for the value column, struct column, and make_constant_metadata_column instead of invoking cudf::detail::copy_bitmask separately at each site. Adjust make_constant_metadata_column’s inputs as needed to accept and use the existing copied mask while preserving current null-mask behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/include/cudf/io/experimental/variant.hpp`:
- Around line 115-137: Update the Doxygen block for the VARIANT encoding
function to document that column_names may contain at most 255 field names and
that exceeding this limit throws std::invalid_argument via the existing
validation. Add the constraint and `@throws` documentation without changing the
implementation.
In `@cpp/src/io/parquet/experimental/variant_encode.cu`:
- Around line 204-221: Decode JSON escape sequences in the quoted-string
handling shared by encoded_field_size and write_field_value before determining
length or writing VARIANT bytes, including \n, \t, \\, escaped quotes, and
\uXXXX to UTF-8. Ensure both functions use the same decoded length and content;
alternatively, explicitly reject backslash-containing string values in both
paths.
- Around line 230-234: Update the integer encoding path around try_parse_int64
so failed parses fall back to FLOAT64 encoding of the original raw value instead
of silently storing INT64 zero. Ensure the parser accepts and correctly
represents INT64_MIN by using unsigned-magnitude or negative-result
accumulation, while preserving INT64 encoding for successfully parsed values.
- Around line 167-181: Require quoted-string inputs to have at least two bytes
before entering the string-handling branch in both encoded_field_size and
write_field_value, changing the existing raw.size_bytes() > 0 guard
consistently. Preserve normal handling for valid quoted strings while preventing
str_len from becoming negative for a lone quote.
- Around line 262-275: Update the value-size accumulation and inclusive scan
producing value_offsets/total_value_bytes to use int64_t, including the relevant
temporary and output types. Before the allocation in the write-values path,
validate total_value_bytes is within size_type’s maximum using
std::numeric_limits<size_type>::max(), and reject or report overflow instead of
allocating an undersized buffer; preserve the existing allocation and write flow
for valid totals.
- Line 504: Route temporary allocations in
cpp/src/io/parquet/experimental/variant_encode.cu at lines 504, 516, 531, 538,
and 426 through cudf::get_current_device_resource_ref() rather than mr,
including d_sorted_to_original, get_json_object results, d_views, value_sizes,
and d_blob; at line 553, use rmm::exec_policy_nosync(stream) without mr. Keep mr
only for returned allocations value_offsets and value_child_data.
- Around line 457-459: Update the make_lists_column call in the surrounding
encoding function to pass the existing stream and memory-resource arguments,
then remove the preceding stream.synchronize() call. Preserve the existing
columns, row count, null count, and null-mask ownership while ensuring
construction stays on the producing stream and resource.
- Around line 490-502: After sorting in the field-name preparation flow,
validate adjacent entries in sort_indices or sorted_names and reject any
duplicate column_names before building metadata or writing values. Return or
propagate the existing validation error mechanism, preserving normal processing
for unique names.
- Around line 479-488: Update the num_rows == 0 branch to pass the caller’s
stream and mr through every make_empty_column, make_lists_column, and
make_structs_column invocation. Ensure all empty metadata/value and returned
struct allocations use the supplied stream and memory resource.
- Around line 127-150: Update exponent handling in parse_float64 to avoid signed
overflow and unbounded per-digit or per-power loops: parse the exponent with
saturation, clamp it to the supported double exponent range, and directly return
or produce 0.0/infinity when the exponent is outside that range. Replace
repeated factor multiplication with a bounded power-of-ten approach or lookup
that preserves correct overflow and underflow behavior, including negative
exponents.
- Around line 546-563: Update the value-offset initialization around
value_offsets and the inclusive_scan to avoid copying from the block-scoped
zero; initialize the first device element with cudaMemsetAsync on the stream
before scanning. Preserve the existing inclusive scan and offset layout while
ensuring the asynchronous operation uses device-owned storage.
- Around line 513-517: Update the JSONPath construction in the variant
extraction loop to handle column names containing `.` or `[` without
interpreting those characters as path syntax. Prefer escaping or quoting each
`column_names[i]` according to the documented JSONPath grammar; if the API
cannot safely represent them, validate and reject such names explicitly before
calling `cudf::get_json_object`.
- Line 435: Add the direct cudf/detail/utilities/cuda_memcpy.hpp include to the
translation unit containing the variant encoding logic, so the
cudf::detail::memcpy_async call is declared without relying on transitive
includes.
In `@cpp/tests/io/experimental/variant_encode_test.cpp`:
- Around line 98-115: Strengthen the scalar conversion assertions in
cpp/tests/io/experimental/variant_encode_test.cpp at lines 98-115 by comparing
extracted values in SingleRowFloat and SingleRowFloatExponent against expected
FLOAT64 columns containing 3.14 and 150.0, respectively, while retaining size
and validity checks. At lines 156-164, compare the extracted result with an
INT64 column containing one null row so JSON-null validity is explicitly
verified.
- Around line 24-40: Extend cpp/tests/io/experimental/variant_encode_test.cpp at
lines 24-40 by adding a helper or test path that constructs and passes a sliced
cudf::strings_column_view to encode_strings_to_variant. Update lines 135-153 to
include non-ASCII UTF-8 strings and cases on both sides of the short/long string
encoding boundary. Expand the tests at lines 231-243 with enough rows to
exercise encoding and extraction across multiple CUDA blocks.
- Around line 6-18: Update the includes in the variant encode test to add
cudf_test/cudf_gtest.hpp, the direct header defining cudf::strings_column_view,
and the standard headers defining std::unique_ptr and int64_t. Keep the existing
includes and avoid relying on transitive dependencies.
---
Nitpick comments:
In `@cpp/src/io/parquet/experimental/variant_encode.cu`:
- Around line 588-591: Update the surrounding encoding flow to copy
input_null_mask only once, then reuse that device buffer for the value column,
struct column, and make_constant_metadata_column instead of invoking
cudf::detail::copy_bitmask separately at each site. Adjust
make_constant_metadata_column’s inputs as needed to accept and use the existing
copied mask while preserving current null-mask behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b1d660af-01aa-4ca7-b9bb-ff7f64d8c3f1
📒 Files selected for processing (5)
cpp/CMakeLists.txtcpp/include/cudf/io/experimental/variant.hppcpp/src/io/parquet/experimental/variant_encode.cucpp/tests/CMakeLists.txtcpp/tests/io/experimental/variant_encode_test.cpp
Comment on lines
+167
to
+181
| __device__ size_type encoded_field_size(cudf::string_view raw) | ||
| { | ||
| if (raw == cudf::string_view("null", 4)) { return 1; } | ||
| if (raw == cudf::string_view("true", 4) || raw == cudf::string_view("false", 5)) { return 1; } | ||
|
|
||
| if (raw.size_bytes() > 0 && raw.data()[0] == '"') { | ||
| // JSON string: strip surrounding quotes | ||
| size_type str_len = static_cast<size_type>(raw.size_bytes()) - 2; | ||
| if (str_len <= 63) { return 1 + str_len; } // SHORT_STRING | ||
| return 1 + 4 + str_len; // LONG_STRING (1 hdr + 4-byte len + bytes) | ||
| } | ||
|
|
||
| if (is_float_number(raw)) { return 9; } // header + 8-byte double | ||
| return 9; // header + 8-byte int64 | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Guard the quoted-string length; a one-byte " value causes a negative length and an out-of-bounds memcpy.
encoded_field_size and write_field_value both enter the string branch when raw.size_bytes() > 0 && raw.data()[0] == '"'. Both then compute str_len = raw.size_bytes() - 2. If raw is a single " byte, str_len becomes -1.
The consequences are:
encoded_field_sizereturns0, so the row's allocated size is too small.write_field_valuewrites a header byte and then callscuda::std::memcpy(out, str_start, str_len)withstr_lenconverted tosize_t, which isSIZE_MAX.
Require at least two bytes before entering the string branch.
🛡️ Proposed guard
- if (raw.size_bytes() > 0 && raw.data()[0] == '"') {
+ if (raw.size_bytes() >= 2 && raw.data()[0] == '"') {
// JSON string: strip surrounding quotes
size_type str_len = static_cast<size_type>(raw.size_bytes()) - 2;Apply the same change at line 204 in write_field_value so both functions agree.
Also applies to: 204-221
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/io/parquet/experimental/variant_encode.cu` around lines 167 - 181,
Require quoted-string inputs to have at least two bytes before entering the
string-handling branch in both encoded_field_size and write_field_value,
changing the existing raw.size_bytes() > 0 guard consistently. Preserve normal
handling for valid quoted strings while preventing str_len from becoming
negative for a lone quote.
Source: Coding guidelines
Comment on lines
+204
to
+221
| if (raw.size_bytes() > 0 && raw.data()[0] == '"') { | ||
| auto const* str_start = raw.data() + 1; | ||
| size_type str_len = static_cast<size_type>(raw.size_bytes()) - 2; | ||
|
|
||
| if (str_len <= 63) { | ||
| *out++ = | ||
| static_cast<uint8_t>(basic_type::SHORT_STRING) | (static_cast<uint8_t>(str_len) << 2u); | ||
| cuda::std::memcpy(out, str_start, str_len); | ||
| return out + str_len; | ||
| } | ||
| // LONG_STRING | ||
| *out++ = make_prim_header(primitive_type::LONG_STRING); | ||
| uint32_t len32 = static_cast<uint32_t>(str_len); | ||
| cuda::std::memcpy(out, &len32, 4); | ||
| out += 4; | ||
| cuda::std::memcpy(out, str_start, str_len); | ||
| return out + str_len; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Decode JSON escape sequences before writing VARIANT string bytes.
write_field_value copies the raw bytes between the quotes verbatim. get_json_object returns the source JSON text, so escape sequences remain encoded. For the input row {"a":"he said \"hi\""} the encoder writes the literal bytes he said \"hi\" into the VARIANT value. The VARIANT specification requires unescaped UTF-8 string bytes.
The same defect affects \n, \t, \\, and \uXXXX sequences. encoded_field_size also over-counts, because the decoded length is shorter than the raw length.
This PR adds string support, so this path is the primary feature. Either decode the escapes in both encoded_field_size and write_field_value, or document and reject rows that contain a backslash inside a string value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/io/parquet/experimental/variant_encode.cu` around lines 204 - 221,
Decode JSON escape sequences in the quoted-string handling shared by
encoded_field_size and write_field_value before determining length or writing
VARIANT bytes, including \n, \t, \\, escaped quotes, and \uXXXX to UTF-8. Ensure
both functions use the same decoded length and content; alternatively,
explicitly reject backslash-containing string values in both paths.
Comment on lines
+230
to
+234
| *out++ = make_prim_header(primitive_type::INT64); | ||
| auto parsed = try_parse_int64(raw); | ||
| int64_t ival = parsed.has_value() ? *parsed : int64_t{0}; | ||
| cuda::std::memcpy(out, &ival, sizeof(int64_t)); | ||
| return out + sizeof(int64_t); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not silently encode an unparsable integer as 0.
try_parse_int64 returns nullopt on overflow. Line 232 then substitutes 0. Two valid JSON inputs become the wrong value with no diagnostic:
-9223372036854775808: the guard at line 92 rejects it, because the magnitude exceedsINT64_MAX. The row encodes0.- Any integer above
INT64_MAX, for example99999999999999999999. The row encodes0.
Encode the value as a FLOAT64 primitive when the integer parse fails. That preserves magnitude and matches the fallback other JSON readers use. Handle INT64_MIN correctly by accumulating the magnitude as uint64_t or by accumulating a negative result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/io/parquet/experimental/variant_encode.cu` around lines 230 - 234,
Update the integer encoding path around try_parse_int64 so failed parses fall
back to FLOAT64 encoding of the original raw value instead of silently storing
INT64 zero. Ensure the parser accepts and correctly represents INT64_MIN by
using unsigned-magnitude or negative-result accumulation, while preserving INT64
encoding for successfully parsed values.
Comment on lines
+24
to
+40
| // Encode a vector of JSON strings with the given column names. | ||
| std::unique_ptr<cudf::column> encode(std::vector<std::string> const& json_rows, | ||
| std::vector<std::string> const& col_names, | ||
| std::vector<bool> const& valid = {}) | ||
| { | ||
| std::unique_ptr<cudf::column> input_col; | ||
| if (valid.empty()) { | ||
| cudf::test::strings_column_wrapper w(json_rows.begin(), json_rows.end()); | ||
| input_col = w.release(); | ||
| } else { | ||
| cudf::test::strings_column_wrapper w(json_rows.begin(), json_rows.end(), valid.begin()); | ||
| input_col = w.release(); | ||
| } | ||
| cudf::strings_column_view scv{input_col->view()}; | ||
| std::vector<std::string> names(col_names); | ||
| return cudf::io::parquet::experimental::encode_strings_to_variant(scv, names); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add the required encoder input coverage.
The suite does not exercise sliced input, non-ASCII UTF-8, both sides of the string-encoding boundary, or multi-block row counts.
cpp/tests/io/experimental/variant_encode_test.cpp#L24-L40: Add a helper or test path that passes a slicedcudf::strings_column_viewtoencode_strings_to_variant.cpp/tests/io/experimental/variant_encode_test.cpp#L135-L153: Add non-ASCII UTF-8 strings and boundary cases at the short/long-string transition.cpp/tests/io/experimental/variant_encode_test.cpp#L231-L243: Add enough rows to execute the encoding and extraction paths across multiple CUDA blocks.
As per coding guidelines, tests must cover sliced columns, boundary and multi-block sizes, and non-ASCII UTF-8 for string tests.
📍 Affects 1 file
cpp/tests/io/experimental/variant_encode_test.cpp#L24-L40(this comment)cpp/tests/io/experimental/variant_encode_test.cpp#L135-L153cpp/tests/io/experimental/variant_encode_test.cpp#L231-L243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/tests/io/experimental/variant_encode_test.cpp` around lines 24 - 40,
Extend cpp/tests/io/experimental/variant_encode_test.cpp at lines 24-40 by
adding a helper or test path that constructs and passes a sliced
cudf::strings_column_view to encode_strings_to_variant. Update lines 135-153 to
include non-ASCII UTF-8 strings and cases on both sides of the short/long string
encoding boundary. Expand the tests at lines 231-243 with enough rows to
exercise encoding and extraction across multiple CUDA blocks.
Source: Coding guidelines
Comment on lines
+98
to
+115
| TEST_F(EncodeStringsToVariantTest, SingleRowFloat) | ||
| { | ||
| auto variant = encode({R"({"f":3.14})"}, {"f"}); | ||
|
|
||
| auto floats = extract_float64(variant->view(), "$.f"); | ||
| // Check that we get a non-null row | ||
| EXPECT_EQ(floats->null_count(), 0); | ||
| EXPECT_EQ(floats->size(), 1); | ||
| } | ||
|
|
||
| TEST_F(EncodeStringsToVariantTest, SingleRowFloatExponent) | ||
| { | ||
| auto variant = encode({R"({"f":1.5e2})"}, {"f"}); | ||
|
|
||
| auto floats = extract_float64(variant->view(), "$.f"); | ||
| EXPECT_EQ(floats->null_count(), 0); | ||
| EXPECT_EQ(floats->size(), 1); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert scalar conversion results.
The tests currently accept incorrect float values and incorrect JSON-null validity.
cpp/tests/io/experimental/variant_encode_test.cpp#L98-L115: Compare the extracted values with expectedFLOAT64columns for3.14and150.0.cpp/tests/io/experimental/variant_encode_test.cpp#L156-L164: Compare the result with anINT64column that has one null row.
📍 Affects 1 file
cpp/tests/io/experimental/variant_encode_test.cpp#L98-L115(this comment)cpp/tests/io/experimental/variant_encode_test.cpp#L156-L164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/tests/io/experimental/variant_encode_test.cpp` around lines 98 - 115,
Strengthen the scalar conversion assertions in
cpp/tests/io/experimental/variant_encode_test.cpp at lines 98-115 by comparing
extracted values in SingleRowFloat and SingleRowFloatExponent against expected
FLOAT64 columns containing 3.14 and 150.0, respectively, while retaining size
and validity checks. At lines 156-164, compare the extracted result with an
INT64 column containing one null row so JSON-null validity is explicitly
verified.
3 tasks
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
github-actions
Bot
added
libcudf
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
(DRAFT)
Checklist