[feature](iceberg) Support reading Iceberg variant from Parquet#63192
[feature](iceberg) Support reading Iceberg variant from Parquet#63192eldenmoon wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
eldenmoon
force-pushed
the
codex/iceberg-v3-variant
branch
from
cab85b8 to
e9e3bfd
Compare
|
run buildall |
|
/review |
|
OpenCode automated review failed and did not complete. Error: Review step was skipped (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary:
I found one blocking issue in the added regression test: the local single-BE copy path does not match the file_path used by the local TVF, so the test can fail in the exact environment that branch is intended to support.
Critical checkpoint conclusions:
- Goal: add Iceberg/Parquet VARIANT read support, including shredded projection. The implementation and regression coverage mostly target that goal, but the new regression test has a path setup bug.
- Scope: the production changes are focused on Parquet schema parsing, variant reconstruction, column pruning, and Iceberg type mapping.
- Concurrency/lifecycle: no new shared mutable concurrent state or non-obvious lifecycle ownership issue found in the reviewed PR diff.
- Configuration/compatibility: no new config items or persisted storage-format changes found; FE/BE type mapping paths for Iceberg VARIANT are updated.
- Parallel paths: Hive, Iceberg, and local Parquet pruning paths were considered; the test issue is distinct from production pruning logic.
- Tests: regression coverage was added, but the local-file staging logic can make the new test fail before validating the feature.
- Observability/performance: added ParquetReadColumnPaths profile string is useful for validating pruning; no blocking observability or hot-path issue found beyond the test blocker.
User focus: no additional user-provided review focus was present.
There was a problem hiding this comment.
I found a correctness blocker in the new shredded VARIANT pruning logic. The implementation prunes the unshredded value leaf whenever a matching typed_value path exists, but Iceberg shredded VARIANT can still carry residual/unrepresentable values for that field in value, so queries can silently return NULL or partial objects for those rows.
Critical checkpoint conclusions:
- Goal/test: the PR adds Iceberg v3 VARIANT reading and pruning tests, but the tests only cover fully typed shredded fields and do not prove residual fallback correctness.
- Scope/focus: the change is mostly focused on Parquet/Iceberg VARIANT support.
- Concurrency/lifecycle/config: no new concurrency, non-trivial lifecycle, or config behavior found in the reviewed PR diff.
- Compatibility: adds new type mapping; no storage-format persistence changes found.
- Parallel paths: the same pruning issue exists in both standalone/Hive Parquet and Iceberg Parquet helper paths.
- Tests: missing mixed shredded/residual cases where a selected typed path is absent or has an incompatible type in
typed_valuebut exists invalue. - Observability/performance: profile string helps inspect selected leaves; no additional blocking observability issue found.
- Data correctness: blocking issue below can cause incorrect query results after column pruning.
No additional user-provided focus points were present.
eldenmoon
force-pushed
the
codex/iceberg-v3-variant
branch
from
e9e3bfd to
5fe9ca5
Compare
|
run buildall |
|
/review |
1 similar comment
|
/review |
There was a problem hiding this comment.
Reviewed the full PR with the Doris code-review checklist. I found a blocking correctness issue in nested VARIANT pruning for deeper accesses under shredded fields. Goal/test: the PR adds Iceberg/Parquet VARIANT read support and includes a regression for top-level shredded field reads, but it does not cover deeper access through a shredded field residual. Scope: the change is focused, but the duplicated Hive/Iceberg pruning helper needs the same fix. Concurrency/lifecycle/config/transaction/persistence: no new concurrency, lifecycle, config, transaction, storage-format write, or persistence concerns found. Parallel paths: the same issue exists in both Hive/local Parquet and Iceberg Parquet helpers. Tests: existing tests cover unshredded reads and top-level shredded field pruning, but should add a mixed residual case such as v['metric']['x'] where metric is shredded and some rows store an object in typed_value.metric.value. Observability: the new profile string is useful for validating selected leaves. User focus: no additional user-provided review focus was supplied.
| } else { | ||
| std::vector<std::vector<std::string>> child_paths { | ||
| std::vector<std::string>(path.begin() + 1, path.end())}; | ||
| found_typed_path = extract_nested_column_ids_by_name(*typed_child, child_paths, |
There was a problem hiding this comment.
This handles v['metric'], but deeper accesses under a shredded field still prune the wrong residual. For a schema like typed_value.metric { value, typed_value int64 }, the access path for v['metric']['x'] reaches this branch with typed_child == metric and path.size() > 1. metric is parsed as a regular struct, not TYPE_VARIANT, so extract_nested_column_ids_by_name(*typed_child, {['x']}, ...) finds no child named x and returns false. The code then falls back to top-level v.value, even though the Parquet VARIANT shredding spec stores non-int/object residuals for metric in v.typed_value.metric.value and forbids duplicating shredded field keys in top-level v.value. That makes deeper projections/filters silently return missing/null for rows where metric is an object stored in the field-level residual. Please treat these unannotated shredded field groups (value/typed_value) as variant-like during recursive pruning, and add a regression that projects a subpath from a shredded field whose residual is stored in typed_value.<field>.value.
There was a problem hiding this comment.
Fixed in latest push. VARIANT user keys now use exact matching while structural Parquet fields still use structural lookup. The pruning helper now treats unannotated value/typed_value shredded field groups as variant-like, so v['metric']['x'] selects v.typed_value.metric.value instead of top-level v.value. Added profile assertions for the deeper residual path.
| } else { | ||
| std::vector<std::vector<std::string>> child_paths { | ||
| std::vector<std::string>(path.begin() + 1, path.end())}; | ||
| found_typed_path = extract_nested_column_ids_by_name(*typed_child, child_paths, |
There was a problem hiding this comment.
Same distinct deeper-path issue as the Hive/local helper, separate from the already-discussed top-level residual question. When Iceberg pruning sees an access like v['metric']['x'] and metric is a shredded field group with value plus typed_value, the recursive call treats metric as a plain struct and returns false because there is no child named x. The fallback then selects top-level v.value, but Iceberg/Parquet shredded field residuals for metric live in v.typed_value.metric.value; top-level v.value must not contain that shredded key. Iceberg scans can therefore lose residual object values for deeper projections or filters under a shredded field. Please make the recursive pruning recognize the unannotated value/typed_value shredded-field layout and cover this with an Iceberg/local regression case.
There was a problem hiding this comment.
Fixed in latest push. VARIANT user keys now use exact matching while structural Parquet fields still use structural lookup. The pruning helper now treats unannotated value/typed_value shredded field groups as variant-like, so v['metric']['x'] selects v.typed_value.metric.value instead of top-level v.value. Added profile assertions for the deeper residual path.
There was a problem hiding this comment.
Review result: request changes.
Critical checkpoint conclusions:
- Goal/test coverage: the PR adds Iceberg VARIANT Parquet reading, type mapping, pruning observability, and regression/unit coverage. The main scenario is covered, but a case-sensitive key path is not covered and currently regresses correctness.
- Scope/focus: the change is focused on Iceberg/Parquet VARIANT support, though duplicated Hive/Iceberg pruning helpers carry the same issue.
- Concurrency/lifecycle: no new shared mutable state, threads, locks, or static initialization hazards found in the reviewed paths.
- Configuration/compatibility: no new configs or storage-format writes; this is a reader/type-mapping change. Mixed files with non-VARIANT types continue through existing paths.
- Parallel paths: the Hive/local and Iceberg Parquet pruning paths were both reviewed; both have the same case-sensitivity bug and are commented separately.
- Error handling/memory: Status returns in the new reader path are generally propagated; no ignored Status or untracked large persistent allocation issue found beyond the correctness issue raised.
- Data correctness: blocking issue found: shredded VARIANT field lookup lowercases user path components, so distinct keys such as
aandAcan be pruned/read as the same field. - Observability/performance: the profile leaf-path observable is useful; no additional blocker found.
User focus: no additional user-provided review focus was specified.
eldenmoon
force-pushed
the
codex/iceberg-v3-variant
branch
from
5fe9ca5 to
fa098c0
Compare
|
run buildall |
|
/review |
TPC-H: Total hot run time: 29804 ms |
TPC-H: Total hot run time: 29785 ms |
FE UT Coverage ReportIncrement line coverage |
There was a problem hiding this comment.
I found two additional correctness issues in the Parquet VARIANT reconstruction path.
Critical checkpoint conclusions:
- Goal/test: The PR adds Iceberg v3 Parquet VARIANT reading and shredded column pruning, with regression and FE unit coverage for the common unshredded/shredded object paths. However, valid shredded array layouts and non-finite shredded floats are not covered and can decode incorrectly or fail.
- Scope/focus: The implementation is mostly focused on the new reader/pruning behavior, though the duplicated Hive/Iceberg pruning helper logic remains a maintainability risk rather than a blocker.
- Concurrency/lifecycle: The reviewed changes are per-reader/per-query state and do not introduce new shared mutable state, locks, background threads, or special static lifecycle dependencies.
- Config/compatibility: No new config items or storage-format writes are introduced. The change reads a standard Parquet/Iceberg format; mixed-version compatibility concerns are limited to reader capability.
- Parallel paths: Hive/local and Iceberg pruning paths are both updated. Standalone Parquet uses the Hive-style name pruning helper.
- Conditional checks: The main conditional logic around shredded
value/typed_valuefollows the Parquet Variant Shredding spec for object fields, but the array element check currently misses a spec-valid layout. - Tests/results: Existing tests cover top-level object shredding, deeper residual paths, case-sensitive keys, and profile observability. Missing coverage remains for arrays whose element group omits
value, and for NaN/Inf in shredded float/double typed values. - Observability: The added
ParquetReadColumnPathsprofile string is useful for pruning verification and appears lightweight. - Transactions/persistence/data writes: Not applicable; this is read-path only.
- FE/BE variable passing: Iceberg type mapping and access-path rewriting are updated for VARIANT; no additional thrift variable propagation issue found.
- Performance: The JSON reconstruction path is inherently allocation-heavy but limited to VARIANT decoding. No additional performance blocker found beyond the correctness issues below.
No additional user-provided review focus was specified.
| std::string element_json; | ||
| bool element_present = false; | ||
| if (element_schema != nullptr && find_child_idx(*element_schema, "value") >= 0) { | ||
| RETURN_IF_ERROR(_variant_to_json(*element_schema, values[i], &metadata, &element_json, |
There was a problem hiding this comment.
This only recognizes shredded array elements when the element group contains a value child, but the Parquet Variant Shredding spec allows array element to omit value when elements are fully shredded as a specific typed_value type. For a valid schema like typed_value (LIST) -> element { optional binary typed_value (STRING) }, this branch falls through to _field_to_json(*element_schema, ...) and reconstructs each element as an object such as {"typed_value":"x"} instead of "x". Please treat an element group with either value or typed_value as a shredded variant element and add coverage for the value-omitted array-element layout.
TPC-DS: Total hot run time: 170353 ms |
TPC-DS: Total hot run time: 169901 ms |
eldenmoon
force-pushed
the
codex/iceberg-v3-variant
branch
from
fa098c0 to
bf0c548
Compare
|
/review |
|
run buildall |
There was a problem hiding this comment.
I found additional blocking compatibility issues for valid Parquet VARIANT shredding layouts that omit value when data is fully represented by typed_value. Existing review threads already cover non-finite typed floats, typed array elements, case-sensitive keys, and value/residual pruning, so I did not duplicate those.
Critical checkpoints:
- Goal/test: the PR aims to read Iceberg/Parquet VARIANT, including shredded layouts, and adds local TVF regression coverage, but coverage does not include typed-value-only top-level or nested shredded field groups.
- Scope/focus: the change is focused, but the schema/pruning logic is stricter than the Parquet shredding layout it is trying to support.
- Concurrency/lifecycle/config/transactions/persistence: no new concurrency, lifecycle, config, transaction, or persistence concerns found in the reviewed paths.
- Parallel paths: Hive/local and Iceberg pruning have duplicated logic; both need the typed-value-only fix.
- Compatibility/data correctness: current code rejects or prunes away valid typed-value-only shredded data, causing scan failure or null/missing results.
- Tests: existing tests cover unshredded/shredded happy paths and several pruning observables, but miss the typed-value-only layouts described in the inline comments.
- Observability/performance: no additional observability or performance blocker found beyond the added profile string being used by tests.
- User focus: no additional user-provided review focus was present.
| has_value = child.physical_type == tparquet::Type::BYTE_ARRAY; | ||
| } | ||
| } | ||
| if (!has_metadata || !has_value) { |
There was a problem hiding this comment.
This rejects a valid fully-shredded VARIANT schema where the top-level value field is omitted and all data is represented by typed_value. The Parquet shredding layout makes the fallback value optional for fully shredded values, and the reader below already has code paths that can decode wrappers with typed_value but no value. With this check, such files fail schema parsing before reading. Please accept metadata plus either value or typed_value (validating their physical/logical shape), and add coverage for a top-level typed-value-only shredded variant.
| } | ||
|
|
||
| bool is_shredded_variant_field(const FieldSchema& field_schema) { | ||
| return find_child_by_structural_name(field_schema, "value") != nullptr && |
There was a problem hiding this comment.
This only treats a nested shredded field as variant-like when both value and typed_value are present. A fully shredded field may omit the fallback value, for example typed_value.metric { typed_value { x ... } }. For an access like v['metric']['x'], extract_variant_nested_column_ids() recurses into metric, this predicate returns false, and the generic struct path looks for a child named x next to typed_value, so it returns false and falls back to top-level v.value instead of selecting v.typed_value.metric.typed_value.x. Please also recognize typed_value-only shredded field groups and add a pruning/read regression for that layout.
TPC-H: Total hot run time: 29314 ms |
TPC-DS: Total hot run time: 168835 ms |
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#63192 Problem Summary: Doris could not read Iceberg v3 VARIANT columns from Parquet files. This change maps Iceberg VARIANT to Doris VARIANT, validates the Parquet VARIANT wrapper shape from the VariantShredding spec, decodes unshredded metadata/value encoding, reads shredded typed_value columns, and prunes shredded Parquet leaf columns for accessed variant paths with profile observability. It keeps selected typed-only shredded projections on native Parquet typed columns when residual value columns are not selected, including top-level, nested, root-array, nested-array, typed-only, value-only residual, typed-map key lookup, binary, temporal, UUID, optional top-level, nullable ARRAY/MAP layouts, and Iceberg field-id pruning. It falls back to row-wise reconstruction only for complex or selected-residual layouts, while preserving binary, binary-array, complex binary arrays, residual null, UUID, decimal-array, typed-array null element, empty-object, root-null, out-of-order residual object, and user fields named value semantics. It also keeps full VARIANT projection separate from predicate subpath pruning, supports element paths produced by variant array explode, forces a root read for dynamic VARIANT element access, and explicitly rejects Iceberg VARIANT writes because this PR only implements read support. ### Release note Support reading Iceberg v3 VARIANT Parquet columns, including shredded typed_value column pruning and binary/UUID VARIANT values. Non-finite floating-point VARIANT values remain unsupported and return explicit errors. Writing Iceberg VARIANT columns is rejected with an explicit unsupported error. ### Check List (For Author) - Test: Regression test / Unit Test / Manual test / Static analysis - Unit Test: ./run-be-ut.sh --run --filter='ParquetVariantReaderTest.*:IcebergReaderCreateColumnIdsTest.*:NestedColumnAccessHelperTest.*' (97 tests passed: 68 ParquetVariantReaderTest, 7 IcebergReaderCreateColumnIdsTest, 22 NestedColumnAccessHelperTest) - Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest,org.apache.doris.planner.IcebergTableSinkTest,org.apache.doris.planner.IcebergMergeSinkTest (55 tests passed: 51 PruneNestedColumnTest, 1 IcebergTableSinkTest, 3 IcebergMergeSinkTest; Maven reactor/checkstyle succeeded) - Regression test: performance regression coverage is included in regression-test/suites/external_table_p0/tvf/test_local_tvf_iceberg_variant.groovy; not run locally in this worktree because no local Doris cluster/output BE+FE runtime is available. - Manual test: env PATH=/tmp/codex-clang-format:$PATH build-support/clang-format.sh - Manual test: env PATH=/tmp/codex-clang-format:$PATH build-support/check-format.sh - Manual test: git diff --cached --check - Static analysis: CLANG_TIDY_BINARY=/mnt/disk1/claude-max/ldb_toolchain16/bin/clang-tidy build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN (attempted earlier in this PR loop; local clang-tidy could not analyze the changed files because existing be/src/util/jni-util.h static_assert(false) diagnostics are promoted to errors before producing actionable changed-line diagnostics.) - Behavior changed: Yes. Doris can read Iceberg v3 VARIANT Parquet columns, supports spec-compliant binary and UUID VARIANT values, validates wrapper fields more strictly, avoids row-wise reconstruction for supported typed-only shredded projections, preserves residual binary/null values, typed-array null elements, nullable complex pruning, user value-only fields, and complex array elements with binary leaves, preserves full VARIANT projection when predicates read typed subpaths, handles dynamic root element access, optional top-level VARIANT, root/nested variant-array element paths, typed-map key lookup paths, and rejects Iceberg VARIANT writes explicitly. - Does this need documentation: No
eldenmoon
force-pushed
the
codex/iceberg-v3-variant
branch
from
6587ec2 to
4dacd58
Compare
|
run buildall |
|
/review |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#63192 Problem Summary: Doris could not read Iceberg v3 VARIANT columns from Parquet files. This change maps Iceberg VARIANT to Doris VARIANT, validates the Parquet VARIANT wrapper shape from the VariantShredding spec, decodes unshredded metadata/value encoding, reads shredded typed_value columns, and prunes shredded Parquet leaf columns for accessed variant paths with profile observability. It keeps selected typed-only shredded projections on native Parquet typed columns when residual value columns are not selected, including Iceberg field-id pruning, and falls back to row-wise reconstruction only for complex or selected-residual layouts. It also keeps full VARIANT projection separate from predicate subpath pruning, supports element paths produced by variant array explode, forces a root read for dynamic VARIANT element access, and explicitly rejects Iceberg VARIANT writes because this PR only implements read support. ### Release note Support reading Iceberg v3 VARIANT Parquet columns, including shredded typed_value column pruning and binary/UUID VARIANT values. Non-finite floating-point VARIANT values remain unsupported and return explicit errors. Writing Iceberg VARIANT columns is rejected with an explicit unsupported error. ### Check List (For Author) - Test: Regression test / Unit Test / Manual test - Unit Test: ./run-be-ut.sh --run --filter='ParquetVariantReaderTest.*:NestedColumnAccessHelperTest.*:DeltaByteArrayDecoderTest.*:IcebergReaderCreateColumnIdsTest.*' (107 tests passed) - Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest,org.apache.doris.planner.IcebergTableSinkTest,org.apache.doris.planner.IcebergMergeSinkTest (55 tests passed: 51 PruneNestedColumnTest, 1 IcebergTableSinkTest, 3 IcebergMergeSinkTest; Maven reactor succeeded) - Regression test: performance regression coverage is included in regression-test/suites/external_table_p0/tvf/test_local_tvf_iceberg_variant.groovy, including profile assertions that typed-only projections increment VariantDirectTypedValueReadRows and keep VariantRowWiseReadRows at 0. Not run locally in this worktree because no local Doris cluster/output BE+FE runtime is available. - Manual test: PATH=/mnt/disk6/common/ldb_toolchain_toucan/bin:$PATH build-support/clang-format.sh - Manual test: PATH=/mnt/disk6/common/ldb_toolchain_toucan/bin:$PATH build-support/check-format.sh - Manual test: git diff --check - Behavior changed: Yes. Doris can read Iceberg v3 VARIANT Parquet columns, supports spec-compliant binary and UUID VARIANT values, validates wrapper fields more strictly, avoids row-wise reconstruction for supported typed-only shredded projections, preserves residual binary/null values, typed-array null elements, nullable complex pruning, user value-only fields, and complex array elements with binary leaves, preserves full VARIANT projection when predicates read typed subpaths, handles dynamic root element access, optional top-level VARIANT, root/nested variant-array element paths, typed-map key lookup paths, and rejects Iceberg VARIANT writes explicitly. - Does this need documentation: No
eldenmoon
force-pushed
the
codex/iceberg-v3-variant
branch
from
4dacd58 to
e1dce69
Compare
|
run buildall |
|
/review |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review summary for head 4dacd58. I reviewed the current diff with the existing inline review threads as known context and did not find an additional distinct issue that was not already covered by those threads. The existing VARIANT/Parquet correctness comments should still be resolved before merge if they remain applicable.\n\nCritical checkpoint conclusions:\n- Goal/test coverage: The PR aims to add Iceberg/Parquet VARIANT read support, nested pruning, write rejection for unsupported VARIANT writes, and focused unit/regression coverage. Tests were added, but existing review threads identify remaining uncovered edge cases.\n- Scope: The change is large but focused on Iceberg/Parquet VARIANT support and related pruning/planning paths.\n- Concurrency/lifecycle/config: I did not identify new concurrency, lifecycle, or configuration risks in the reviewed diff.\n- Compatibility/write behavior: Iceberg VARIANT reads are introduced while writes are rejected in FE planning; BE/FE type mapping paths were checked at a high level.\n- Parallel paths: Local Parquet, Hive, Iceberg, FE collector, and TVF paths were reviewed for consistency; no new distinct inconsistency beyond existing threads was found.\n- Data correctness/error handling: The highest risk remains VARIANT reconstruction/pruning correctness, already covered by existing inline threads.\n- Performance/observability: No additional blocking performance or observability issue found beyond the cost inherent in row-wise fallback paths.\n\nUser focus: No additional user-provided review focus was specified.
There was a problem hiding this comment.
I found one remaining blocking correctness issue in the Parquet VARIANT reader.
Critical checkpoint conclusions:
- Goal/test: the PR aims to read Iceberg/Parquet VARIANT, including shredded typed values and nested pruning. The current tests do not cover optional complex typed_value dispatch, and that valid path can still fail reader creation.
- Scope/focus: the change is generally focused on VARIANT read/pruning support, but this reader factory dispatch gap affects the new VARIANT struct-reader path.
- Concurrency/lifecycle/config/compatibility: no new concurrency, lifecycle, configuration, or storage-format compatibility concern found beyond the read-path correctness issue.
- Parallel paths: the same factory dispatch is shared by local/Hive/Iceberg Parquet reads, so the fix should apply centrally.
- Error handling/data correctness: the issue can turn valid optional complex typed shards into scalar readers with invalid physical column indexes, causing scan failure instead of returning data.
- Test coverage: add coverage for optional complex typed_value shards (struct/list/map) selected through VARIANT pruning/full projection.
- Observability/performance: no separate observability or performance blocker found.
- User focus: no additional user-provided review focus was specified.
Please address the inline issue before merging.
| @@ -115,7 +1667,7 @@ Status ParquetColumnReader::create(io::FileReaderSPtr file, FieldSchema* field, | |||
| size_t total_rows = row_group.num_rows; | |||
| if (field->data_type->get_primitive_type() == TYPE_ARRAY) { | |||
| std::unique_ptr<ParquetColumnReader> element_reader; | |||
There was a problem hiding this comment.
The factory still dispatches ARRAY/MAP/STRUCT using field->data_type->get_primitive_type() without unwrapping nullable, so the new VARIANT struct-reader path can still send a valid optional complex typed_value child into the scalar-reader branch. For example, a VARIANT schema with optional group typed_value { optional int64 a; } selected by v['a'] reaches the top synthetic struct reader, then recurses into the typed_value child whose data_type is Nullable(Struct(...)); this skips the STRUCT branch and falls through with the group field's physical_column_index == -1. This is distinct from the earlier optional top-level VARIANT thread because the top synthetic struct is now non-nullable, but its optional complex children remain nullable. Please dispatch ARRAY/MAP/STRUCT on remove_nullable(field->data_type) as well, and add coverage for optional complex typed_value struct/list/map shards.
There was a problem hiding this comment.
I found one additional correctness issue in the new VARIANT row-wise typed array path. Existing threads already cover several other blocking VARIANT pruning/decoding gaps, including nullable dispatch, residual handling, binary nested arrays, metadata validation, and FE full-column access collection; I did not repeat those.
Critical checkpoint conclusions: the PR goal is to support Iceberg/Parquet VARIANT reads and pruning; the implementation is broad and has tests, but the current code still misses valid typed-array cases and therefore does not fully satisfy data correctness. The change is focused on VARIANT support but touches several parallel BE/FE paths, and the existing review context shows multiple parallel paths still need alignment. I did not identify new concurrency, lifecycle, configuration, transaction, or persistence concerns in this pass. FE-BE protocol compatibility appears intentionally read-focused with Iceberg VARIANT writes rejected in FE. Test coverage has been expanded, but it is still missing the empty/all-null typed decimal array case below and the existing already-raised cases. User focus: no additional user-provided review focus was specified.
| case TYPE_ARRAY: | ||
| value->field = field; | ||
| fill_variant_field_info(value); | ||
| fill_variant_leaf_type_info(type, value); |
There was a problem hiding this comment.
This direct typed-array branch derives base_scalar_type_id and dimensions from the row value before applying schema metadata. For a valid typed leaf such as typed_value: array<decimal(18,2)>, rows containing [] or [null] have no non-null element, so variant_util::get_field_info() leaves the scalar type as INVALID_TYPE/Nothing; fill_variant_leaf_type_info() only fills precision/scale and never restores the array element type or dimensions from field_schema.data_type. Those rows are then inserted with different/invalid VARIANT type metadata from non-empty rows, losing the Parquet typed-array fidelity. This is distinct from the existing decimal-array JSON round-trip thread because it happens in the direct typed leaf path even without serializing through JSON. Please fill the array scalar type/dimensions from the typed schema when field-derived info is invalid, and add coverage for empty and all-null typed decimal arrays.
TPC-H: Total hot run time: 30734 ms |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#63192 Problem Summary: Doris could not read Iceberg v3 VARIANT columns from Parquet files. This change maps Iceberg VARIANT to Doris VARIANT, validates the Parquet VARIANT wrapper shape from the VariantShredding spec, decodes unshredded metadata/value encoding, reads shredded typed_value columns, and prunes shredded Parquet leaf columns for accessed variant paths with profile observability. It keeps selected typed-only shredded projections on native Parquet typed columns when residual value columns are not selected, including Iceberg field-id pruning below VARIANT typed_value trees, and falls back to row-wise reconstruction only for complex or selected-residual layouts. It also keeps full VARIANT projection separate from predicate subpath pruning, supports element paths produced by variant array explode, forces a root read for dynamic VARIANT element access, and explicitly rejects Iceberg VARIANT writes because this PR only implements read support. ### Release note Support reading Iceberg v3 VARIANT Parquet columns, including shredded typed_value column pruning and binary/UUID VARIANT values. Non-finite floating-point VARIANT values remain unsupported and return explicit errors. Writing Iceberg VARIANT columns is rejected with an explicit unsupported error. ### Check List (For Author) - Test: Regression test / Unit Test / Manual test - Unit Test: ./run-be-ut.sh --run --filter='ParquetVariantReaderTest.*:NestedColumnAccessHelperTest.*:DeltaByteArrayDecoderTest.*:IcebergReaderCreateColumnIdsTest.*' (109 tests passed) - Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.SlotTypeReplacerTest,org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest (53 tests passed; Maven reactor succeeded) - Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest,org.apache.doris.planner.IcebergTableSinkTest,org.apache.doris.planner.IcebergMergeSinkTest (55 tests passed earlier in this worktree) - Regression test: performance regression coverage is included in regression-test/suites/external_table_p0/tvf/test_local_tvf_iceberg_variant.groovy, including profile assertions that typed-only projections increment VariantDirectTypedValueReadRows and keep VariantRowWiseReadRows at 0. Not run locally in this worktree because no local Doris cluster/output BE+FE runtime is available. - Manual test: PATH=/mnt/disk6/common/ldb_toolchain_toucan/bin:$PATH build-support/clang-format.sh - Manual test: PATH=/mnt/disk6/common/ldb_toolchain_toucan/bin:$PATH build-support/check-format.sh - Manual test: git diff --check - Manual test: CLANG_TIDY_BINARY=/mnt/disk6/common/ldb_toolchain_toucan/bin/clang-tidy-16 build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN (blocked by local analyzer errors in included be/src/util/jni-util.h after the changed-line warnings in this patch were fixed) - Behavior changed: Yes. Doris can read Iceberg v3 VARIANT Parquet columns, supports spec-compliant binary and UUID VARIANT values, validates wrapper fields more strictly, avoids row-wise reconstruction for supported typed-only shredded projections, preserves residual binary/null values, typed-array null elements, nullable complex pruning, user value-only fields, and complex array elements with binary leaves, preserves full VARIANT projection when predicates read typed subpaths, handles dynamic root element access, optional top-level VARIANT, root/nested variant-array element paths, typed-map key lookup paths, Iceberg field-id paths below VARIANT typed_value trees, and rejects Iceberg VARIANT writes explicitly. - Does this need documentation: No
eldenmoon
force-pushed
the
codex/iceberg-v3-variant
branch
from
e1dce69 to
f78e331
Compare
|
run buildall |
|
/review |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#63192 Problem Summary: Doris could not read Iceberg v3 VARIANT columns from Parquet files. This change maps Iceberg VARIANT to Doris VARIANT, validates the Parquet VARIANT wrapper shape from the VariantShredding spec, decodes unshredded metadata/value encoding, reads shredded typed_value columns, and prunes shredded Parquet leaf columns for accessed variant paths with profile observability. It keeps selected typed-only shredded projections on native Parquet typed columns when residual value columns are not selected, including Iceberg field-id pruning below VARIANT typed_value trees, and falls back to row-wise reconstruction only for complex or selected-residual layouts. It also keeps full VARIANT projection separate from predicate subpath pruning, supports element paths produced by variant array explode, forces a root read for dynamic VARIANT element access, and explicitly rejects Iceberg VARIANT writes because this PR only implements read support. ### Release note Support reading Iceberg v3 VARIANT Parquet columns, including shredded typed_value column pruning and binary/UUID VARIANT values. Non-finite floating-point VARIANT values remain unsupported and return explicit errors. Writing Iceberg VARIANT columns is rejected with an explicit unsupported error. ### Check List (For Author) - Test: Regression test / Unit Test / Manual test - Unit Test: ./run-be-ut.sh --run --filter='ParquetVariantReaderTest.*:IcebergReaderCreateColumnIdsTest.*:NestedColumnAccessHelperTest.*' (99 tests passed) - Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.SlotTypeReplacerTest (2 tests passed) - Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.SlotTypeReplacerTest,org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest,org.apache.doris.planner.IcebergTableSinkTest,org.apache.doris.planner.IcebergMergeSinkTest (57 tests passed; Maven reactor succeeded) - Regression test: performance regression coverage is included in regression-test/suites/external_table_p0/tvf/test_local_tvf_iceberg_variant.groovy, including profile assertions that typed-only projections increment VariantDirectTypedValueReadRows and keep VariantRowWiseReadRows at 0. Not run locally in this worktree because no local Doris cluster/output BE+FE runtime is available. - Manual test: env PATH=/tmp/codex-clang-format:$PATH build-support/clang-format.sh - Manual test: env PATH=/tmp/codex-clang-format:$PATH build-support/check-format.sh - Manual test: git diff --check - Static analysis: CLANG_TIDY_BINARY=/mnt/disk1/claude-max/ldb_toolchain16/bin/clang-tidy-16 build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN was attempted. The first checked file reported no warnings; later files were blocked by pre-existing analyzer errors from included be/src/util/jni-util.h static_assert(false) branches. - Behavior changed: Yes. Doris can read Iceberg v3 VARIANT Parquet columns, supports spec-compliant binary and UUID VARIANT values, validates wrapper fields more strictly, avoids row-wise reconstruction for supported typed-only shredded projections, preserves residual binary/null values, typed-array null elements, nullable complex pruning, user value-only fields, and complex array elements with binary leaves, preserves full VARIANT projection when predicates read typed subpaths, handles dynamic root element access, optional top-level VARIANT, root/nested variant-array element paths, typed-map key lookup paths, Iceberg field-id paths below VARIANT typed_value trees, and rejects Iceberg VARIANT writes explicitly. - Does this need documentation: No
eldenmoon
force-pushed
the
codex/iceberg-v3-variant
branch
from
f78e331 to
798ea04
Compare
|
run buildall |
|
/review |
1 similar comment
|
/review |
|
run buildall |
TPC-DS: Total hot run time: 169296 ms |
There was a problem hiding this comment.
I found two additional correctness issues in the BE Parquet VARIANT paths that are distinct from the existing review threads.
Checkpoint conclusions:
- Goal/test: the PR aims to add Iceberg/Parquet VARIANT read, pruning, and write mapping support with BE/unit/regression coverage. The current tests cover many paths, but they do not prove residual non-JSON-native scalar type fidelity or metadata selection for root non-STRUCT typed_value residuals.
- Scope/clarity: the change is broad but generally focused on VARIANT support; the remaining issues are localized to VariantMap decoding and nested column id selection.
- Concurrency/lifecycle: no new concurrency, locking, or special lifecycle issue found in the reviewed paths.
- Configuration/compatibility: no new config found. This does involve external Parquet/Iceberg format compatibility; the issues below affect valid VARIANT encodings and can produce incorrect decoded values or missing metadata.
- Parallel paths: Hive/Iceberg/local pruning and JSON/VariantMap decode paths were checked against existing comments; I avoided duplicating already-known nullable/pruning/array/binary/object-offset issues.
- Tests/results: added coverage is substantial, but missing for the two scenarios commented inline. Regression outputs appear deterministic where reviewed.
- Observability/performance: no additional observability or obvious hot-path performance blocker found beyond existing comments.
- Transaction/persistence/data writes: no FE transaction or persisted metadata issue found in this pass; Iceberg write support concerns already have existing context.
- User focus: no additional user-provided review focus was specified.
Given the data-correctness risk for valid Parquet VARIANT encodings, I recommend changes before merge.
| return Status::OK(); | ||
| } | ||
|
|
||
| std::string json; |
There was a problem hiding this comment.
This VariantMap path still round-trips non-binary Parquet VARIANT primitives through JSON, which loses the primitive type carried by the VARIANT value encoding. For example, a residual root value encoded with primitive header 8/9/10 for DECIMAL reaches decode_primitive_to_variant_map(), decode_primitive() emits only a JSON number, and parse_json_to_variant_map() reconstructs it as a normal numeric field without the original decimal precision/scale metadata; the same applies to date/time/timestamp/UUID headers that are emitted as JSON numbers/strings. This is distinct from the existing typed-array JSON round-trip comments because it affects residual scalar VARIANT values on the VariantMap path. Please decode these primitive headers directly into FieldWithDataType instead of reparsing JSON, and add coverage for residual DECIMAL and temporal/UUID scalar values, including array elements.
| if (typed_value != nullptr) { | ||
| const auto typed_value_type = remove_nullable(typed_value->data_type); | ||
| if (typed_value_type->get_primitive_type() != TYPE_STRUCT) { | ||
| typed_result = extract_variant_typed_nested_column_ids(*typed_value, {path}, |
There was a problem hiding this comment.
This non-STRUCT root typed_value branch drops the needs_metadata signal returned by extract_variant_typed_nested_column_ids(). For a root typed array/map that contains a shredded residual child, e.g. typed_value: array<struct<metric: { value: binary }>> queried as v[0]['metric']['x'], the recursive extraction selects the nested value leaf and returns needs_metadata=true, but this branch never adds the top-level metadata column. Row-wise reconstruction then tries to decode the residual value without the inherited VARIANT metadata and can fail or decode from missing metadata. This is distinct from the existing root typed-array selection thread because the typed leaf is selected, but its required metadata is not. Please mirror the STRUCT branch by adding add_variant_metadata(variant_field, column_ids) when the non-STRUCT typed result needs metadata, with coverage for root typed array/map residual children.
There was a problem hiding this comment.
I found one additional issue. The main feature goal is to add Iceberg/Parquet VARIANT read support and nested pruning; the PR adds broad BE/FE tests, but this remaining FE collector case can still force full VARIANT reads for unused CTE producer outputs. The change is large and mostly focused on VARIANT support, but many correctness edge cases are already tracked in existing review threads. Concurrency/lifecycle: no new relevant shared-state concurrency or special lifecycle issue found in the reviewed FE collector path. Configuration/compatibility: no new config; Iceberg VARIANT write compatibility is already covered by an existing thread. Parallel paths: BE/Hive/Iceberg/local variant paths have many already-known parallel-path comments; I did not duplicate them. Test coverage: broad regression/unit coverage was added, but this unused CTE producer pruning scenario is not covered. User focus: no additional user-provided review focus was specified.
| @@ -230,11 +231,15 @@ public Void visitLogicalGenerate(LogicalGenerate<? extends Plan> generate, State | |||
| public Void visitLogicalProject(LogicalProject<? extends Plan> project, StatementContext context) { | |||
| AccessPathExpressionCollector exprCollector | |||
| = new AccessPathExpressionCollector(context, allSlotToAccessPaths, false); | |||
There was a problem hiding this comment.
This uses an empty global allSlotToAccessPaths as proof that the current project is the query result, but inner producer projects can also be visited with no propagated demand. For example, WITH c AS (SELECT v FROM variant_tbl) SELECT 1 FROM c visits the CTE consumer first, records no access path for v, then visits the producer project with this map still empty. This branch records a root [v] path even though the outer query never uses v, so the scan is forced to read the full VARIANT column unnecessarily. Please distinguish true root/result projection demand from a producer/subquery with no consumer demand, and add coverage for an unused VARIANT CTE/subquery output.
2 participants
What problem does this PR solve?
Issue Number: N/A
Related PR: #63192
Problem Summary: Doris could not read Iceberg v3 VARIANT columns from Parquet files. This change maps Iceberg VARIANT to Doris VARIANT, validates the Parquet VARIANT wrapper shape from the VariantShredding spec, decodes unshredded metadata/value encoding, reads shredded typed_value columns, and prunes shredded Parquet leaf columns for accessed variant paths with profile observability. It keeps selected typed-only shredded projections on native Parquet typed columns when residual value columns are not selected, including top-level, nested, root-array, nested-array, typed-only, value-only residual, typed-map key lookup, binary, temporal, UUID, optional top-level, nullable ARRAY/MAP layouts, and Iceberg field-id pruning. It falls back to row-wise reconstruction only for complex or selected-residual layouts, while preserving binary, binary-array, complex binary arrays, residual null, UUID, decimal-array, typed-array null element, empty-object, root-null, out-of-order residual object, and user fields named value semantics. It also keeps full VARIANT projection separate from predicate subpath pruning, supports element paths produced by variant array explode, forces a root read for dynamic VARIANT element access, and explicitly rejects Iceberg VARIANT writes because this PR only implements read support.
Release note
Support reading Iceberg v3 VARIANT Parquet columns, including shredded typed_value column pruning and binary/UUID VARIANT values. Non-finite floating-point VARIANT values remain unsupported and return explicit errors. Writing Iceberg VARIANT columns is rejected with an explicit unsupported error.
Check List (For Author)
Test: Regression test / Unit Test / Manual test / Static analysis
Unit Test: ./run-be-ut.sh --run --filter='ParquetVariantReaderTest.:IcebergReaderCreateColumnIdsTest.:NestedColumnAccessHelperTest.*' (97 tests passed: 68 ParquetVariantReaderTest, 7 IcebergReaderCreateColumnIdsTest, 22 NestedColumnAccessHelperTest)
Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest,org.apache.doris.planner.IcebergTableSinkTest,org.apache.doris.planner.IcebergMergeSinkTest (55 tests passed: 51 PruneNestedColumnTest, 1 IcebergTableSinkTest, 3 IcebergMergeSinkTest; Maven reactor/checkstyle succeeded)
Regression test: performance regression coverage is included in regression-test/suites/external_table_p0/tvf/test_local_tvf_iceberg_variant.groovy; not run locally in this worktree because no local Doris cluster/output BE+FE runtime is available.
Manual test: env PATH=/tmp/codex-clang-format:$PATH build-support/clang-format.sh
Manual test: env PATH=/tmp/codex-clang-format:$PATH build-support/check-format.sh
Manual test: git diff --cached --check
Static analysis: CLANG_TIDY_BINARY=/mnt/disk1/claude-max/ldb_toolchain16/bin/clang-tidy build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN (attempted earlier in this PR loop; local clang-tidy could not analyze the changed files because existing be/src/util/jni-util.h static_assert(false) diagnostics are promoted to errors before producing actionable changed-line diagnostics.)
Behavior changed: Yes. Doris can read Iceberg v3 VARIANT Parquet columns, supports spec-compliant binary and UUID VARIANT values, validates wrapper fields more strictly, avoids row-wise reconstruction for supported typed-only shredded projections, preserves residual binary/null values, typed-array null elements, nullable complex pruning, user value-only fields, and complex array elements with binary leaves, preserves full VARIANT projection when predicates read typed subpaths, handles dynamic root element access, optional top-level VARIANT, root/nested variant-array element paths, typed-map key lookup paths, and rejects Iceberg VARIANT writes explicitly.
Does this need documentation: No