Table to Parquet Variant Encoding - #23400
Conversation
github-actions
Bot
added
libcudf
abigalekim
added
feature request
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds public VARIANT encoding APIs, duplicate column-name validation, expanded string, numeric, floating-point, null, and round-trip tests, and CUDA build registrations for VARIANT encoding and split filtered-join implementations. ChangesVARIANT encoding
Filtered join build refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 7
🧹 Nitpick comments (4)
cpp/src/io/parquet/experimental/variant_encode.cu (2)
268-285: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win1 KiB per-thread local array will spill heavily; the sizes can be streamed instead.
uint32_t field_sizes[256]is 1 KiB of local memory per thread (256 KiB per block) and it recomputes what pass 1 already computed. Since the offsets are a simple prefix sum over the fields, write the offset table in a first loop over fields and the values in a second, without materializing the array.♻️ Suggested restructuring
- // Compute per-field sizes (stack array, safe for N < 256) - uint32_t field_sizes[256]; - for (int i = 0; i < N; i++) { - field_sizes[i] = static_cast<uint32_t>(field_encoded_size(tbl.column(sort_order[i]), row)); - } - - // Write (N+1) field offsets, 4 bytes each (LE) - uint32_t running = 0; - for (int i = 0; i <= N; i++) { - cuda::std::memcpy(p, &running, 4); - p += 4; - if (i < N) { running += field_sizes[i]; } - } + // Write (N+1) field offsets, 4 bytes each (LE) + uint32_t running = 0; + for (int i = 0; i <= N; i++) { + cuda::std::memcpy(p, &running, 4); + p += 4; + if (i < N) { + running += static_cast<uint32_t>(field_encoded_size(tbl.column(sort_order[i]), row)); + } + }🤖 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 268 - 285, Remove the per-thread field_sizes array and its size-computation pass in the variant encoding flow. In the offset-writing loop, compute each field’s encoded size directly via field_encoded_size(tbl.column(sort_order[i]), row) and accumulate running; keep the existing second loop for writing field values in sorted order.Source: Coding guidelines
30-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing direct includes for used standard symbols.
std::numeric_limits(Lines 407, 515),std::pair(Lines 295, 348) andstd::unique_ptrare used without<limits>,<utility>,<memory>.As per coding guidelines: "include headers directly for every used symbol without unused or incorrectly styled includes."
📦 Proposed include additions
`#include` <algorithm> `#include` <cstdint> +#include <limits> +#include <memory> `#include` <numeric> `#include` <string> +#include <utility> `#include` <vector>🤖 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 30 - 34, Add direct standard-library includes for every used symbol in this translation unit: include the headers declaring std::numeric_limits, std::pair, and std::unique_ptr alongside the existing includes. Keep the existing include set and style, adding only the required <limits>, <utility>, and <memory> headers.Source: Coding guidelines
cpp/tests/io/experimental/variant_encode_test.cpp (2)
158-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
meta_childis unused. Either drop it or add a test that asserts the metadata child bytes directly (which would also cover theoffset_size > 1metadata path inbuild_metadata_blob).🤖 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 158 - 162, Remove the unused meta_child helper, or add a test using it to assert metadata child bytes produced by build_metadata_blob, including the offset_size > 1 path. Prefer removal unless direct metadata-byte coverage is needed.
176-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCoverage gaps: sliced inputs, multi-block sizes, and non-ASCII UTF-8.
All string cases are single-block (≤3 rows) and ASCII, and no test exercises a sliced input column — which is where the
column_device_viewoffset handling andcopy_bitmaskpath inencode_strings_to_variantare most likely to break. Adding a >256-row case (block_size is 256) plus a multi-byte UTF-8 string near the 63/64-byte short/long boundary would cover the boundary logic inwrite_field_value.As per coding guidelines: "Tests must cover empty inputs, nulls, sliced columns, boundary and multi-block sizes, and non-ASCII UTF-8 for string tests". Want me to draft these cases?
🤖 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 176 - 286, Expand EncodeStringsToVariantTest to cover the requested gaps: add a sliced strings column and verify values/nulls, add more than 256 rows to exercise multi-block encoding, and add non-ASCII UTF-8 inputs around the 63/64-byte short/long boundary with expected round-trip or encoded results. Preserve the existing empty, null, type-error, and current encoding tests.Source: Coding guidelines
🤖 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`:
- Line 130: Update the throws documentation for the variant API around
strings_column_view to name the input parameter and specify cudf::logic_error,
matching the exception surfaced during strings_column_view construction; remove
the stale std::invalid_argument and strings references.
- Around line 137-162: Update the encode_variant documentation to list FLOAT32
and FLOAT64 among the supported column types, and document that exceeding the 2
GiB encoded-output limit throws std::overflow_error rather than
std::invalid_argument. Keep the existing std::invalid_argument conditions
limited to unsupported types, too many columns, and mismatched column_names
size.
In `@cpp/src/io/parquet/experimental/variant_encode.cu`:
- Line 397: Update the temporary device allocations d_val_sizes,
d_meta_template, and d_sort_order to use cudf::get_current_device_resource_ref()
instead of the caller-provided mr; keep mr only for allocations returned from
the function.
- Around line 378-381: Update the make_empty_list lambda and the corresponding
empty-list assembly sites to pass the active stream and memory resource to every
make_empty_column and make_lists_column call. Apply the same stream/MR
propagation at all four referenced locations, preserving the existing column
types and structure while eliminating default-stream and default-resource usage.
- Around line 348-362: Update make_constant_list_buffers and
object_encode_write_kernel to perform row-count, metadata-size, and offset
arithmetic in a sufficiently wide integer type before multiplication, preventing
size_type overflow during allocation and device writes. Ensure the offsets
representation and sequence remain valid for large products, or explicitly
reject inputs that cannot fit the existing INT32 offsets.
In `@cpp/tests/io/experimental/variant_encode_test.cpp`:
- Around line 6-18: Add the required cudf_test/cudf_gtest.hpp include to
variant_encode_test.cpp alongside the existing cudf_test headers, without
changing the test implementation.
In `@cpp/tests/io/experimental/variant_extract_test.cpp`:
- Around line 964-987: The test named FloatEncodeMatchesApacheBytes does not
verify encoded bytes, only round-trips values through extract_variant_field.
Either add a direct comparison of the encoded value-child bytes against the
Apache fixture using the existing build_object_value pattern, or rename the test
and comments to describe round-trip validation instead.
---
Nitpick comments:
In `@cpp/src/io/parquet/experimental/variant_encode.cu`:
- Around line 268-285: Remove the per-thread field_sizes array and its
size-computation pass in the variant encoding flow. In the offset-writing loop,
compute each field’s encoded size directly via
field_encoded_size(tbl.column(sort_order[i]), row) and accumulate running; keep
the existing second loop for writing field values in sorted order.
- Around line 30-34: Add direct standard-library includes for every used symbol
in this translation unit: include the headers declaring std::numeric_limits,
std::pair, and std::unique_ptr alongside the existing includes. Keep the
existing include set and style, adding only the required <limits>, <utility>,
and <memory> headers.
In `@cpp/tests/io/experimental/variant_encode_test.cpp`:
- Around line 158-162: Remove the unused meta_child helper, or add a test using
it to assert metadata child bytes produced by build_metadata_blob, including the
offset_size > 1 path. Prefer removal unless direct metadata-byte coverage is
needed.
- Around line 176-286: Expand EncodeStringsToVariantTest to cover the requested
gaps: add a sliced strings column and verify values/nulls, add more than 256
rows to exercise multi-block encoding, and add non-ASCII UTF-8 inputs around the
63/64-byte short/long boundary with expected round-trip or encoded results.
Preserve the existing empty, null, type-error, and current encoding tests.
🪄 Autofix (Beta)
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: b598d0f0-c250-42d9-9dc1-842f541f6dd6
📒 Files selected for processing (6)
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.cppcpp/tests/io/experimental/variant_extract_test.cpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… into ak/json-to-variant-infra
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/src/io/parquet/experimental/variant_encode.cu`:
- Around line 306-310: Move the duplicate column-name validation loop before the
zero-row early return in the variant encoding flow, ensuring it runs even when
build_metadata_blob is skipped. Preserve the existing column_names and
sort_order comparison and invalid_argument behavior.
🪄 Autofix (Beta)
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: 28893e3f-8ebd-4c5d-985b-d87829a47072
📒 Files selected for processing (2)
cpp/src/io/parquet/experimental/variant_encode.cucpp/tests/io/experimental/variant_encode_test.cpp
| CUDF_CUDA_TRY(cudaGetLastError()); | ||
| } | ||
|
|
||
| auto [val_offsets_col, total_val_bytes] = cudf::strings::detail::make_offsets_child_column( |
There was a problem hiding this comment.
Could you please confirm whether val_offsets_col is guaranteed to be INT32 here?
make_offsets_child_column() can return INT64 offsets when large strings are enabled or the configured threshold is reached, but below we read the data as size_type/INT32.
If INT64 is possible here, could we build checked INT32 offsets for this < 2 GiB API, or handle the actual offset type before launching the kernel? The same pattern appears in encode_variant below.
There was a problem hiding this comment.
I've added a CUDF_EXPECTS that verifies the type of the column so that its guaranteed to be less than 2GB.
There was a problem hiding this comment.
Thanks for adding the check!
The code currently calls cudf::strings::detail::make_offsets_child_column. I was wondering whether it should instead use cudf::detail::make_offsets_child_columnin both paths.
The strings specific helper can switch to INT64 offsets at the configurable LIBCUDF_LARGE_STRINGS_THRESHOLD. The new type check prevents those offsets from being read as INT32, but it could still reject a valid sub-2-GiB VARIANT result when that threshold is lowered.
The generic helper always creates INT32 offsets and performs the scan and overflow check in 64-bit. Would that be a better fit here?
@mhaseeb123 - Need your suggestion as I am not familiar with the code base. Please let us know if the current implementation is fine or changing it to cudf::detail::make_offsets_child_column would be better?
There was a problem hiding this comment.
my opinion is that cudf::detail::make_offsets_child_column would be better so that we have more control over the behavior of the code, and I'm happy to change it.
mhaseeb123
left a comment
mhaseeb123
left a comment
There was a problem hiding this comment.
First pass. Flushing comments so far
|
Please also update the PR description to briefly say what the new API does or what it's for instead of simply pointing to the linked issue :) |
|
I updated the PR description & fixed comments, so I am re-requesting reviewers! |
…m/cudf into ak/json-to-variant-infra
|
After discussion with @vuule, we have decided to split the implementation of the table => Variant API function and the JSON => variant API function. I will link the PR to the JSON => Variant API function here when it is completed. |
abigalekim
added
3 - Ready for Review
|
The JSON string column to Variant column PR is here: #23614 |
abigalekim
changed the title
3 participants
Description
Adds the new API function
cudf::io::parquet::experimental::encode_variantmentioned in #23251. Currently this code only supports scalar, non-nested variant values. This PR is mainly to enable the infrastructure to support the rest of the Parquet Variant JSON converter support.encode_variant(table, column_names, stream, mr)converts a flat cuDF table (output fromcudf::read_json) into a Parquet Variant object column. Each row contains a self contained variant object. Supported column types are INT8/16/32/64, FLOAT32/64, STRING, and EMPTY (all-null).Boolean support (#23276) is now merged. There is support for Boolean types in this PR.
Checklist