Repository navigation
Conversation
b186aeb to
f8d6cb9
Compare
c5c54dc to
a16f004
Compare
9086570 to
63e8a1a
Compare
0ddd3c1 to
1a0b24f
Compare
|
@pitrou @emkornfield Can you help to review the API design? I’ve tried to keep the code concise, but this is still a huge PR. I’d like to focus on the overall API design for now, and then I’ll break this PR down into several smaller ones. I include brief introductions to each module in the PR description. |
There was a problem hiding this comment.
Hi @HuaHuaY , nice work on this, the layered architecture (compiled plans, RAII rollback, strict/relaxed validation) is well thought out. One general observation: the current implementation does a fair amount of per-row work that could be amortized across batches either in the current set of planned PRs or a follow-up. I've left inline comments on the three spots I noticed. None are correctness issues or blockers.
| const auto source = DecodeSourceScalar(value); | ||
| if (source != nullptr && CanAttemptCast(*source, *storage_type_)) { | ||
| auto casted = ::arrow::compute::Cast(::arrow::Datum(source), storage_type_, | ||
| ::arrow::compute::CastOptions::Safe(), ctx_); |
There was a problem hiding this comment.
This decodes each row into a scalar and calls compute::Cast individually. For large arrays this is a bottleneck since scalar Cast has non-trivial per-call overhead (kernel lookup, ExecContext dispatch, output scalar allocation).
A follow-up optimization could accumulate decoded source scalars into a temporary array grouped by source type, then batch-cast in one shot and scatter results back. This would amortize kernel dispatch and let the cast kernels use their SIMD/batch paths.
There was a problem hiding this comment.
This is an improvement, but since the optimization feels somewhat complex and independent of the basic implementation of Variant, I think we can leave it for a future PR.
|
|
||
| std::vector<uint32_t> value_offsets = field_offsets; | ||
| std::ranges::sort(value_offsets); | ||
| if (std::ranges::adjacent_find(value_offsets) != value_offsets.end()) { |
There was a problem hiding this comment.
This copies all field offsets into a new vector, sorts, and checks for duplicates -- per object, per row. For typical small objects (<10 fields) it's fine, but at scale a reusable scratch buffer (or a linear scan for small n, switching to sort only above a threshold) would reduce per-row allocation pressure.
There was a problem hiding this comment.
This sorted array will be used for subsequent binary searches, so I don’t think it can be easily eliminated without compromising the time complexity. Perhaps we could achieve some optimization by using arrow::internal::SmallVector.
|
PyArrow 25.0.1 can read files with the Once C++ has the writer ready, please patch up Python. Thank you! |
|
Looking forward to this PR being merged. 👀 |
Apologies for the delay in responding to the review comments on the PR. I plan to resume pushing for the PR's merge starting this weekend. #50979 and #51266 are PRs I opened that are derived from this PR (and this PR depends on them); meanwhile, we also need to wait for #50810 to be merged. However, I will first address the review comments that are unrelated to those PRs. |
|
@HuaHuaY All the PRs seem to have merged. Can you rebase? Thanks. |
I rebased in my local environment and I will force-push the changes in a batch after addressing some review comments. Sorry for the lack of GitHub updates over my past two weeks public holidays. |
You don't have to apologize for being on holiday :) |
|
@wgtmac @emkornfield @qzyu999 Thank you for all your review comments, which have helped improve the API’s usability, performance, and other aspects. It certainly took a significant amount of time to address them. Unless I’ve overlooked anything, every single comment has either resulted in a code change or been addressed with an explanation as to why no change was made. |
There was a problem hiding this comment.
🟡 Changes recommended
An undefined Meson dependency breaks Parquet configuration, while Variant field ordering, timestamp coercion, visibility handling, and unbounded recursive validation risk incorrect output, rejection of valid files, and stack exhaustion.
6 open issues
Propagate ancestor visibility masks during Variant validation · New Keep timestamp annotation consistent with coercion conversion · New Preserve storage-field order when constructing Variant nodes · New Bound recursive Variant validation depth to prevent stack exhaustion · New Undefined parquet_test_dep breaks Parquet-enabled Meson configuration · New Validate complete storage shape for optional object fields · New
What changed in this PR
Adds C++ Parquet Variant support, connecting Arrow’s extension type to Parquet reading, writing, validation, and shredded-value reconstruction.
Changes:
- Extends Variant storage types and Parquet schema mappings for shredded values.
- Adds builders, decoding views, validation, and shredding/unshredding APIs.
- Enables reader/writer validation by default and adds unit tests, interoperability tests, and build integration.
| File | Description |
|---|---|
| cpp/src/parquet/variant/validate.h | Declares strict and read-compatible validation. |
| cpp/src/parquet/variant/validate.cc | Implements nested, visibility-aware validation plans. |
| cpp/src/parquet/variant/validate_test.cc | Tests encoded metadata and nested validation. |
| cpp/src/parquet/variant/unshred.h | Declares the reconstruction API. |
| cpp/src/parquet/variant/unshred.cc | Reconstructs values while preserving metadata and nulls. |
| cpp/src/parquet/variant/unshred_test.cc | Tests reconstruction, slices, and invalid storage. |
| cpp/src/parquet/variant/type_test.cc | Tests supported and rejected storage layouts. |
| cpp/src/parquet/variant/test_util_internal.h | Declares shared Variant test helpers. |
| cpp/src/parquet/variant/test_util_internal.cc | Implements construction and round-trip helpers. |
| cpp/src/parquet/variant/slot_internal.h | Defines shared slot-processing plans. |
| cpp/src/parquet/variant/slot_internal.cc | Processes primitive, object, and list slots. |
| cpp/src/parquet/variant/shred.h | Declares target-based shredding. |
| cpp/src/parquet/variant/parquet_testing_test.cc | Adds interoperability fixture tests. |
| cpp/src/parquet/variant/meson.build | Registers tests and public headers. |
| cpp/src/parquet/variant/format.h | Defines Variant encoding tags. |
| cpp/src/parquet/variant/format_internal.h | Adds encoding traits and validation helpers. |
| cpp/src/parquet/variant/decoding.h | Declares zero-copy decoding views. |
| cpp/src/parquet/variant/decoding.cc | Implements parsing and recursive validation. |
| cpp/src/parquet/variant/decoding_test.cc | Tests decoding and malformed encodings. |
| cpp/src/parquet/variant/CMakeLists.txt | Registers tests and header installation. |
| cpp/src/parquet/variant/builder.h | Declares value, metadata, and array builders. |
| cpp/src/parquet/variant/binary_view_column_builder_internal.h | Builds binary-view columns with rollback support. |
| cpp/src/parquet/variant/array_internal.h | Declares array-access and bitmap helpers. |
| cpp/src/parquet/variant/array_internal.cc | Implements encoded metadata and nested-array access. |
| cpp/src/parquet/variant/append_table_internal.h | Shares primitive append definitions. |
| cpp/src/parquet/properties.h | Adds default-enabled validation settings. |
| cpp/src/parquet/meson.build | Includes Variant sources and test support. |
| cpp/src/parquet/CMakeLists.txt | Includes Variant sources and test support. |
| cpp/src/parquet/arrow/writer.cc | Validates Variant data before writing. |
| cpp/src/parquet/arrow/schema.cc | Maps shredded schemas and restores storage metadata. |
| cpp/src/parquet/arrow/reader.cc | Adds Variant-specific read validation. |
| cpp/src/parquet/arrow/arrow_reader_writer_test.cc | Uses a valid all-null array fixture. |
| cpp/src/arrow/extension/parquet_variant.h | Adds shredded-storage accessors and documentation. |
| cpp/src/arrow/extension/parquet_variant.cc | Validates layouts and implements accessors. |
| cpp/src/arrow/extension/parquet_variant_test.cc | Tests accessors and read-compatible storage. |
| cpp/src/arrow/extension/meson.build | Registers Variant extension tests. |
| cpp/src/arrow/extension/CMakeLists.txt | Registers Variant extension tests. |
Review effort: Balanced
| if (ctx_->reader_properties->variant_validation_enabled()) { | ||
| PARQUET_CATCH_NOT_OK(variant::ValidateVariants<false>(**out, ctx_->pool)); |
| case ::arrow::TimeUnit::NANO: | ||
| logical_type = LogicalType::Timestamp(utc, LogicalType::TimeUnit::NANOS, | ||
| /*is_from_converted_type=*/false, | ||
| /*force_set_converted_type=*/false); |
| RETURN_NOT_OK(AddChild(type->metadata())); | ||
| RETURN_NOT_OK(AddChild(type->value())); | ||
| RETURN_NOT_OK(AddChild(type->typed_value())); |
| const size_t child_consumed = ParseValue<true>( | ||
| value.substr(values_start + offsets[i]), metadata, /*out=*/nullptr); |
| 'unshred_test.cc', | ||
| 'validate_test.cc', | ||
| ], | ||
| dependencies: parquet_test_dep, |
| if (type->value() == nullptr) { | ||
| return Status::Invalid("Invalid Variant write schema: missing top-level value field"); | ||
| } |
| /*force_set_converted_type=*/true); | ||
| break; | ||
| case ::arrow::TimeUnit::NANO: | ||
| logical_type = LogicalType::Timestamp(utc, LogicalType::TimeUnit::NANOS, |
There was a problem hiding this comment.
The schema keeps the source unit, but the column writer still applies timestamp coercion. This can write microsecond counts under a NANOS annotation.
| const auto source = DecodeSourceScalar(value); | ||
| if (source != nullptr && CanAttemptCast(*source, *storage_type_)) { | ||
| auto casted = ::arrow::compute::Cast(::arrow::Datum(source), storage_type_, | ||
| ::arrow::compute::CastOptions::Safe(), ctx_); |
There was a problem hiding this comment.
A successful safe cast can still round a double to float or decimal. This branch then clears the residual, so unshredding cannot recover the original numeric value.
| std::shared_ptr<ChunkedArray>* out) override { | ||
| RETURN_NOT_OK(ExtensionReader::BuildArray(length_upper_bound, out)); | ||
| if (ctx_->reader_properties->variant_validation_enabled()) { | ||
| PARQUET_CATCH_NOT_OK(variant::ValidateVariants<false>(**out, ctx_->pool)); |
There was a problem hiding this comment.
This validates the child before its ancestors' visibility masks are applied. A required Variant inside a null struct can therefore reject a valid file.
|
|
||
| RETURN_NOT_OK(AddChild(type->metadata())); | ||
| RETURN_NOT_OK(AddChild(type->value())); | ||
| RETURN_NOT_OK(AddChild(type->typed_value())); |
There was a problem hiding this comment.
These calls reorder storage fields, but the writer traverses the Arrow array in its original order. A supported typed_value-first layout can therefore write to the wrong Parquet leaf.
| if constexpr (validate_children) { | ||
| for (uint32_t i = 0; i < num_elements; ++i) { | ||
| const size_t child_consumed = ParseValue<true>( | ||
| value.substr(values_start + offsets[i]), metadata, /*out=*/nullptr); |
There was a problem hiding this comment.
Recursive validation has no nesting limit. Deeply nested Variant bytes can exhaust the stack during default read validation, independently of schema depth.
| // `VariantExtensionType` already validates the general storage shape. Writing is | ||
| // stricter than reading: we keep accepting typed-only Variant schemas on read, | ||
| // but must not write them. | ||
| if (type->value() == nullptr) { |
There was a problem hiding this comment.
This only checks the top-level value field. Read-compatible object layouts can still be written, and row validation cannot enforce schema requirements for empty or hidden objects.
| const size_t payload_size = PrimitivePayloadSize(value, offset, primitive); | ||
| CheckAvailable(value, offset, payload_size, "primitive value"); | ||
|
|
||
| if (internal::IsDecimalVariantPrimitive(primitive)) { |
There was a problem hiding this comment.
Decimal validation checks the scale but not the unscaled value's precision. A signed 128-bit value can exceed the format's 38-digit limit and still pass this check.
| last_metadata = VariantMetadataView::Make(metadata_bytes); | ||
| last_metadata_bytes = metadata_bytes; | ||
| } | ||
| auto value = VariantValueView::Make(internal::BinaryFieldView(*value_array, row), |
There was a problem hiding this comment.
Make only validates the outer value. If a malformed nested value falls back to residual, it is copied without validation and this API returns success.
| 'unshred_test.cc', | ||
| 'validate_test.cc', | ||
| ], | ||
| dependencies: parquet_test_dep, |
There was a problem hiding this comment.
parquet_test_dep is not defined when this subdirectory is entered; the parent initializes it later. Meson configuration therefore fails when Parquet is enabled.
| test_util_internal.cc | ||
| type_test.cc | ||
| unshred_test.cc | ||
| validate_test.cc) |
There was a problem hiding this comment.
The new fixture includes RapidJSON, but this target does not receive its include dependency. The Windows MSVC job fails on rapidjson/document.h.


Rationale for this change
This PR supports:
This PR does not support:
What changes are included in this PR?
variant/format_internal.h:Define some type-related traits and concepts.
variant/builder.h .cc:Add builders for metadata dictionaries, standalone encoded Variant values, unshredded Variant arrays, and value-only arrays. Nested object and list builders share the owner's value buffer. Value-only builders can reuse an existing metadata view or a writable dictionary across rows. Previous discussion: [C++][Parquet] Encoding tools for variant type #46555
variant/decoding.h .cc:Add zero-copy views with shallow parsing and recursive validation.
variant/shred.h .cc:Convert an unshredded
VariantArrayinto a requested primitive, object, or listtyped_valuelayout while retaining incompatible values in residualvalue.variant/unshred.h .cc:Reconstruct encoded Variant values from shredded storage.
variant/validate.h .cc:Validate nested Variant arrays using strict writer rules or read-compatible rules. First locate the Variant-containing subtrees, then validate visible rows while propagating parent validity.
variant/slot_internal.h .cc:Compile shredded storage layouts and process primitive, object, and list slots for validation and unshredding. This is a common module used by
unshredandvalidate.Some code use
template <bool strict>because some schemas are read-compatible but do not support writing. For details, please see apache/parquet-format#591Are these changes tested?
Yes.
Are there any user-facing changes?
ArrowReaderPropertiesaddsset_variant_validation_enabled().ArrowWriterProperties::Builderaddsset_variant_validation_enabled(). Validation is enabled by default.