Additional Unit Tests for Parquet VARIANT Field Extraction - #23036
Conversation
abigalekim
added
tests
|
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:
📝 WalkthroughWalkthroughAdds shared VARIANT metadata enums, updates extraction decoding to use them, and expands tests for malformed data, casting matrices, string boundaries, and invalid input shapes. ChangesVARIANT contract and extraction validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/ok to test 693c014 |
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
I know very little about Parquet variants. Can you help explain these a bit more?
| TEST_F(CastVariantTest, UnsupportedCastTypeThrows) | ||
| { | ||
| auto stream = cudf::test::get_default_stream(); | ||
| std::vector<uint8_t> const val{0x00}; // null primitive — valid list<uint8> input |
There was a problem hiding this comment.
I might be missing context. I don't really know what this means. Is this 0 a value in the Parquet specification that represents null? Is this representing a type id, or a value?
There was a problem hiding this comment.
Is uint8_t a type declared in the Parquet variant spec?
There was a problem hiding this comment.
Hi @bdice these tests are targeting casting from sample raw variant bytes (like the val vector here and other tests). We extract these bytes out from variant rows as col<list<uint8_t>> (one list per extracted field per variant row)
Variant looks like this:
First byte -> header
bits 0-1: basic_type (0=primitive, 1=short_string, 2=object, 3=array)
bits 2-7: value_header (6 bits, meaning depends on basic_type)
For basic_type::primitive (0), the 6-bit value_header is a physical type id (0=null, 1/2=bool true/false, 3-6=int8/16/32/64, 7=float64, 16=long_string, etc. from the enum).
For basic_type::short_string (1), the 6-bit value_header is the string length (0-63 bytes) and the string bytes follow immediately.
In this test, there's just one byte 0x0 saying primitive type but no other info on what type and what data (so it throws). Please correct me if I am wrong @abigalekim
Similarly in the next test, we have 0x1 (indicating short string) but nothing really after so it should decode to a valid but empty ("") value.
There was a problem hiding this comment.
Here's a summary of what's going on in each test that I got from AI:
UnsupportedCastTypeThrows — uses val = {0x00}. Byte 0x00 = basic_type 00 (primitive) + value_header 000000 (primitive_type::null), i.e. a Variant "null" value. This is fine as input, but it's actually a bit of a red herring: cast_variant's type-check happens entirely at compile-time dispatch (cast_variant_fn::operator() with requires(not is_variant_castable<T>) → CUDF_FAIL), based purely on the desired output type (FLOAT64/BOOL8), not on the byte content. Any well-formed list<uint8> row would trigger the same throw — this matches exactly what bdice asked about in review ("Is this 0 a value...type id, or a value?").
ShortStringLengthZero — val = {0x01}. 0x01 = basic_type=01 (short_string), value_header = 0 (length 0). Per decode_string, with btype == short_string, str_len = value_header = 0, so it slices 0 bytes after the header → empty string. This exercises the lower boundary of the 6-bit length field.
ShortStringLengthSixtyThree — header 0xFD = binary 11111101: bits 0-1 = 01 (short_string), bits 2-7 = 111111 = 63 (the max value a 6-bit field can hold, i.e. the largest string that can use the short-string encoding before Parquet must switch to long_string). Followed by 63 'z' payload bytes. This is the correctness boundary check corresponding to 0x01 | (63 << 2) = 0xFD in the test comment.
LongStringLengthZero — val = {0x40, 0x00, 0x00, 0x00, 0x00}. 0x40 = binary 01000000: bits 0-1 = 00 (primitive), bits 2-7 = 010000 = 16 = primitive_type::long_string. Per the spec, a long_string primitive is followed by a 4-byte little-endian length, then that many payload bytes — here the length is 0x00000000 = 0, so no payload follows.
LongStringTruncatedPayloadYieldsNull — val = {0x40, 0x0A, 0x00, 0x00, 0x00, 'a','b','c'}: same long_string header, but the LE length field claims 0x0000000A = 10 bytes of payload while only 3 ("abc") are actually present. This targets the truncation guard in decode_string:
@abigalekim can you please double check this and perhaps add some info from here to the tests so it's easy to review 😄. Perhaps we can add a comment/link to the schema so we can refer to it while reviewing.
There was a problem hiding this comment.
Should we have something like a factory that builds a byte from named enums?
Like replace 0x06 with make_variant_primitive(variant_type_id::INT64)?
Then it’s self-explanatory at the call site.
There was a problem hiding this comment.
Also having a factory would help resolve any ambiguity between big-endian and little-endian encodings. (Are bits 0-1 low or high?)
There was a problem hiding this comment.
I think this is a great idea to have a reusable factory + provides context for (current and future) variant tests.
There was a problem hiding this comment.
Hi, I added small comments describing what each test is, and implemented a reusable factory for writing tests! I think the AI descriptions seem correct.
|
|
||
| TEST_F(CastVariantTest, ShortStringLengthZero) | ||
| { | ||
| // Short string with length 0: header = 0x01 | (0 << 2) = 0x01, no payload. |
There was a problem hiding this comment.
Where do the values 0x01 | (0 << 2) come from?
There was a problem hiding this comment.
This test now uses the factory, but it was the manual construction of the VARIANT short-string value-header byte.
| } | ||
| } | ||
|
|
||
| TEST_F(CastVariantTest, UnsupportedCastTypeThrows) |
There was a problem hiding this comment.
There appears to be a real gap in testing (and likely requiring an implementation change) where cast_variant bypasses the validation for empty inputs.
The implementation returns make_empty_column(desired_type) before it reaches the unsupported-type dispatcher, so cast_variant(empty_list_uint8, FLOAT64) can succeed even though unsupported desired_type values are documented to throw.
There was a problem hiding this comment.
I think this should be its own PR. @vuule thoughts?
There was a problem hiding this comment.
@vuule is OOTO so let's not ping him here 🙂. @abigalekim if you would like to handle this in a separate PR, please open an issue (perhaps with a proposed test/repro) against this comment so it could be tracked
There was a problem hiding this comment.
missed this until now. Yeah, separate PR sounds good. Keep them focused so they move faster.
There was a problem hiding this comment.
@abigalekim Please create an issue for this so we don't loose track.
There was a problem hiding this comment.
I made an issue for this problem #23361!
…f into ak/field-extraction-tests
|
/merge |
|
/ok to test 01e5942 |
The merge of main brought in array-element and floating-point extraction code that still used the pre-rename lowercase enumerators and the variant_basic_type() helper, which this branch had replaced with the shared uppercase enums in variant_spec.hpp and decode_basic_type().
|
/ok to test 8ed7f3b |
|
/ok to test e41f893 |
|
/ok to test 03194b0 |
8 participants
Description
Adds edge-case and error-path tests for the experimental Parquet variant extraction API (cast_variant and get_variant_field). New tests cover unsupported cast types, zero-length and max-length short strings, truncated long-string payloads, and invalid input shapes (non-struct/non-list inputs, wrong child count, and mistyped child columns).
Checklist