[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: 169501 ms |
|
run buildall |
|
/review |
TPC-DS: Total hot run time: 168970 ms |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Found one distinct blocking correctness issue after reviewing the current PR and existing review threads.
Critical checkpoint conclusions:
- Goal/test: The PR aims to add Iceberg/Parquet VARIANT reading, pruning, and related FE access-path propagation. There is broad unit/regression coverage, but the chained dynamic-subscript case below is not covered and can still produce wrong results.
- Scope/clarity: The change is large and cross-cuts BE decoding, FE pruning, and external table plumbing; most additions are focused on VARIANT support, but this increases edge-case risk.
- Concurrency/lifecycle: Existing cached Parquet metadata mutation and reader lifecycle risks are already covered by existing threads; I did not find a new distinct concurrency/lifecycle issue beyond those.
- Config/compatibility: No new config items were introduced. Exposing VARIANT through Iceberg/Parquet has compatibility implications already discussed in existing threads; no additional distinct protocol/storage-format issue found.
- Parallel paths: The issue below is in FE access-path collection and affects scan pruning paths that rely on collected VARIANT paths.
- Conditional checks/error handling: No new distinct unchecked Status/exception issue found in the reviewed code.
- Test coverage/results: Existing tests cover many VARIANT layouts, but the chained dynamic-subscript plus sibling predicate case is missing.
- Observability: No additional observability requirement found for this bug; it is a deterministic pruning correctness issue.
- Transactions/persistence/data writes: The reviewed changes are read/planning focused except Iceberg sink type plumbing; no new distinct transaction/persistence issue found.
- Performance: Several performance/correctness pruning tradeoffs are already covered by existing threads; the issue below is correctness-first.
- User focus: No additional user-provided review focus was specified.
Please fix the inline issue before merge.
| return visit(elementAt, context); | ||
| } else if (first.getDataType().isVariantType() && arguments.size() >= 2) { | ||
| CollectorContext variantRootContext = context.copy(); | ||
| variantRootContext.setCollectVariantRoot(true); |
There was a problem hiding this comment.
This still does not force a full VARIANT read when the dynamic subscript has an outer suffix. For v[cast(id AS string)]['x'], the outer literal access leaves accessPathBuilder = [x]; this branch sets collectVariantRoot, but visitSlotReference() checks the non-empty builder first and records [v, x] instead of a root [v] demand. Since the dynamic key can select any top-level field before applying ['x'], pruning to only x can drop required data and return missing/null values when combined with a sibling predicate such as v['k'] IS NOT NULL. Please clear/override the suffix when a dynamic VARIANT subscript requires a root read, or make collectVariantRoot take precedence over a non-empty builder, and add coverage for a chained dynamic subscript plus a sibling nested predicate.
TPC-H: Total hot run time: 30790 ms |
|
run buildall |
|
/review |
There was a problem hiding this comment.
I found one additional correctness issue that is distinct from the existing review threads.
Critical checkpoint conclusions:
- Goal/test coverage: the PR adds Iceberg/Parquet VARIANT read support, nested access-path pruning, and blocks unsupported Iceberg VARIANT writes. Many positive tests were added, but nullable top-level VARIANT null-check pruning for OLAP is not covered.
- Scope/focus: the change is broad but mostly localized to VARIANT access-path collection and Parquet readers; the flagged issue is in the new VARIANT access propagation behavior.
- Concurrency/lifecycle: existing cached Parquet metadata mutation concurrency is already covered by an existing thread; I found no additional distinct concurrency/lifecycle issue.
- Config/compatibility: no new config item. Iceberg write rejection is explicit; mixed-version/storage-format concerns are otherwise around read support.
- Parallel paths: file/TVF scans normalize NULL/OFFSET pseudo paths, but OLAP scans do not, which creates the distinct issue below.
- Transaction/persistence/data writes: Iceberg write paths reject VARIANT; no FE EditLog or Doris transaction persistence change found.
- FE-BE variable passing: access paths are passed through the existing slot access-path mechanism; the issue is an invalid pseudo VARIANT subpath being passed to OLAP BE.
- Performance/observability: no additional distinct performance or observability blocker beyond already-known review threads.
- User focus: no additional user-provided review focus was supplied.
| public Void visit(Expression expr, CollectorContext context) { | ||
| for (Expression child : expr.children()) { | ||
| child.accept(this, new CollectorContext(context.statementContext, context.bottomFilter)); | ||
| if (child.getDataType().isVariantType() |
There was a problem hiding this comment.
This propagation also turns top-level nullable VARIANT null checks into a fake nested subpath on OLAP scans. For SELECT 1 FROM olap_variant_tbl WHERE v IS NULL, visitIsNull() seeds the builder with NULL; this branch preserves that non-empty builder into the VARIANT child, and visitSlotReference() records [v, NULL] as a VARIANT DATA path. NestedColumnPruning handles VARIANT slots outside the DataTypeAccessTree null-only logic, and visitLogicalOlapScan() does not call normalizeDataSkippingOnlyAccessPaths(), so BE receives NULL as if it were a VARIANT field name rather than a top-level null-map/root requirement. File/TVF scans normalize this pseudo suffix away, which is why this is a distinct OLAP path issue. Please special-case VARIANT IS NULL/IS NOT NULL to record a valid root/null access for OLAP (or normalize these pseudo suffixes before building VARIANT AccessPathInfo) and add coverage for nullable top-level VARIANT null predicates.
There was a problem hiding this comment.
I found one additional data-correctness issue in the current head. Review checkpoint conclusions: Goal: the PR adds Iceberg/Parquet VARIANT read support and access-path pruning, but the current wrapper detection still misclassifies a valid typed-only user-object shape. Scope: the change is mostly focused, but this ambiguity needs tightening. Concurrency/lifecycle: no new concurrency issue beyond the already-known cached-footer mutation thread; this comment is local row-wise decode correctness. Config/compatibility: no new config; write-side VARIANT is intentionally rejected, but read compatibility depends on preserving valid typed object field names. Parallel paths: the standalone VariantMap decoder was tightened for unannotated residual values, but the row-wise reader still has a distinct false-positive wrapper path. Tests: current tests cover value-only and typed_value-only cases discussed in prior threads, but not the combined annotated value plus typed_value user-object case. Observability: existing corruption messages would not make this ambiguity obvious. User focus: no additional user-provided focus was supplied.
| } | ||
| if (child.lower_case_name == "value") { | ||
| if (child.physical_type != tparquet::Type::BYTE_ARRAY) { | ||
| return false; |
There was a problem hiding this comment.
This wrapper check still accepts an ordinary annotated user field named value when the same object also has a typed_value child. A valid typed-only VARIANT object such as {"obj": {"value": "abc", "typed_value": {"x": 1}}} is represented under v.typed_value.obj with a UTF8/STRING value leaf plus a struct typed_value leaf. Row-wise reconstruction calls is_variant_wrapper_field(obj, ...), this branch only checks that value is BYTE_ARRAY, and line 215 returns true because typed_value != nullptr; variant_to_variant_map() then tries to decode the UTF8 bytes from obj.value as Parquet VARIANT residual bytes with inherited metadata, causing corruption or wrong output instead of producing user fields obj.value and obj.typed_value.x. This is distinct from the existing value-only and typed_value-only false-positive threads because the misclassification requires both user children together. Please require value to satisfy is_unannotated_variant_value_field() for wrapper classification (as the pruning helper already does) and add coverage for a typed-only object containing both annotated value and typed_value user fields.
TPC-H: Total hot run time: 31051 ms |
FE UT Coverage ReportIncrement line coverage |
TPC-DS: Total hot run time: 168589 ms |
TPC-H: Total hot run time: 30861 ms |
TPC-DS: Total hot run time: 168942 ms |
|
run buildall |
|
/review |
TPC-DS: Total hot run time: 169711 ms |
TPC-H: Total hot run time: 31234 ms |
|
run buildall |
|
/review |
There was a problem hiding this comment.
I found one additional blocking issue beyond the already-open inline threads. The PR goal appears to be Iceberg/Parquet VARIANT read support with nested access-path pruning while keeping VARIANT writes unsupported. The read-side implementation is broad and has tests, but this write-side guard now rejects a delete-only MERGE that should not write VARIANT values.
Critical checkpoint conclusions:
- Goal/test coverage: read support is covered by new regression and unit tests, but there is no coverage for delete-only MERGE on Iceberg tables containing VARIANT columns.
- Scope/focus: most changes are focused on VARIANT read/pruning; the unconditional MERGE sink write guard is broader than necessary.
- Concurrency/lifecycle/config/compatibility: no new concern found in this additional issue.
- Parallel paths: Iceberg DELETE uses a separate delete sink and remains allowed, while delete-only MERGE is newly rejected; these paths should be consistent.
- Data correctness/transactions: the issue is a behavioral regression at planning/binding time rather than persisted-data corruption.
- Performance/observability: no additional concern found.
- User focus: no additional user-provided review focus was specified.
| if (formatVersion >= 3) { | ||
| schema = IcebergUtils.appendRowLineageFieldsForV3(schema); | ||
| } | ||
| IcebergUtils.validateVariantWriteUnsupported(schema); |
There was a problem hiding this comment.
This rejects delete-only MERGE statements on Iceberg tables that contain a VARIANT column, even though that path only needs to write delete files and does not write VARIANT values. For example, MERGE INTO iceberg_variant_tbl t USING src s ON ... WHEN MATCHED THEN DELETE now fails here because the table schema contains VARIANT, while the semantically similar Iceberg DELETE FROM path still uses IcebergDeleteSink and is not rejected. Please scope the VARIANT write validation to MERGE plans that actually produce data-file rows (UPDATE/INSERT branches), or route delete-only MERGE through the delete-only behavior, and add coverage for a delete-only MERGE against a table with a VARIANT column.
There was a problem hiding this comment.
Reviewed the PR against the Doris code-review checklist, existing inline threads, and the stated focus file. I found one additional blocking correctness issue not covered by the existing review threads.
Critical checkpoint conclusions:
- Goal/test coverage: the PR adds Iceberg/Parquet VARIANT reading and nested access pruning support with extensive tests, but the new map OFFSET-only pruning path is not covered and can crash.
- Scope/clarity: the implementation is broad but generally focused on Parquet VARIANT and nested access paths.
- Concurrency/lifecycle: no new issue beyond the already-known cached Parquet schema mutation thread.
- Configuration/compatibility: no new configuration or storage-format compatibility issue found.
- Parallel paths: the map OFFSET-only issue affects standalone Parquet plus Hive/Iceberg because they delegate to the shared Parquet nested-column helper.
- Special checks/error handling: the failure path reaches SkipReadingReader where get_rep_level()/get_def_level() is fatal instead of returning a query error.
- Tests/results: missing coverage for map_size/cardinality on Parquet MAP columns with access-path pruning enabled.
- Observability/performance: no additional observability concern found; the issue is correctness/crash, not only performance.
- Transactions/persistence/data writes/FE-BE variables: not applicable to the reviewed read-path changes.
User focus: no additional user-provided review focus was specified.
| continue; | ||
| } | ||
|
|
||
| if (field_type == PrimitiveType::TYPE_MAP && i == 0) { |
There was a problem hiding this comment.
This path does not handle the OFFSET-only access emitted for map_size(m) / cardinality(m). For a Parquet MAP slot the FE sends [m, OFFSET]; after the root is stripped, child_paths_by_key contains only OFFSET, so neither KEYS nor VALUES is selected and the resulting column_ids contains only the map root. ParquetColumnReader::create() then builds a MapColumnReader with SkipReadingReader for both key and value children. During MapColumnReader::read_column_data(), the key skip reader is used as the reference reader and get_rep_level() / get_def_level() hits the fatal path, so a simple SELECT map_size(m) FROM parquet_map_tbl can crash instead of reading just the map levels. Please make OFFSET-only MAP access select a real key child reader (similar to the values-only case needing keys) or add a dedicated map-offset reader, and cover root and nested MAP cardinality/map_size for standalone/Hive/Iceberg Parquet.
TPC-H: Total hot run time: 30862 ms |
TPC-DS: Total hot run time: 169684 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, keeps field-level residual values for mixed typed/residual shredded subpaths, supports element paths produced by variant array explode, forces root reads for dynamic and non-slot VARIANT element access, preserves VARIANT generator root reads, explicitly rejects Iceberg VARIANT writes because this PR only implements read support, preserves residual primitive VARIANT scalar types without a JSON round trip, and avoids mutating cached Parquet footer schemas while assigning reader-local column ids. For Iceberg MERGE, the VARIANT write rejection is limited to plans that write data files, so delete-only MERGE can still write position deletes. ### Release note Support reading Iceberg v3 VARIANT Parquet columns, including shredded typed_value column pruning and binary/UUID/primitive residual 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, while delete-only Iceberg MERGE on tables that contain VARIANT columns remains allowed because it only writes delete files. ### Check List (For Author) - Test: Regression test / Unit Test / Manual test - Unit Test: ./run-be-ut.sh --run --filter='ParquetVariantReaderTest.DecodeResidualPrimitiveToVariantMapPreservesScalarTypes:ParquetVariantReaderTest.DecodePrimitiveCoverageExtras:ParquetVariantReaderTest.DecodeResidualBinaryToVariantMap:ParquetVariantReaderTest.RejectNonFiniteDoublePrimitive' (4 tests passed) - Unit Test: ./run-be-ut.sh --run --filter='NestedColumnAccessHelperTest.*' (26 tests passed) - Unit Test: ./run-be-ut.sh --run --filter='ParquetVariantReaderTest.*:NestedColumnAccessHelperTest.*:IcebergReaderCreateColumnIdsTest.*' (104 tests passed) - Unit Test: ./run-be-ut.sh --run --filter=ParquetVariantReaderTest.* (74 tests passed) - Unit Test: ./run-be-ut.sh --run --filter='ParquetVariantReaderTest.RejectVariantSchemaWithAnnotatedMetadataChild:ParquetVariantReaderTest.RejectVariantSchemaWithAnnotatedValueChild:ParquetVariantReaderTest.RejectVariantSchemaWithNonBinaryValueChild:ParquetVariantReaderTest.ParseTypedOnlyVariantSchemaWithoutTopLevelValue' (4 tests passed) - Unit Test: ./run-be-ut.sh --run --filter='HiveReaderCreateColumnIdsTest.*' (7 tests passed) - Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest#testNestedVariantInsideStructCollectsSubPath (1 test passed) - Unit Test: ./run-fe-ut.sh --run 'org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest#testVariantWholeColumnWithSiblingSubPathAccessPath' (1 test passed) - Unit Test: ./run-fe-ut.sh --run 'org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest#testVariantWholeNonSlotExpressionWithPredicateAccessPath,org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest#testVariantDynamicElementAtNonSlotExpressionWithPredicateAccessPath,org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest#testVariantLiteralElementAtNonSlotExpressionKeepsSubPath,org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest#testExplodeVariantWholeOutputWithPredicateAccessPath,org.apache.doris.nereids.rules.rewrite.SlotTypeReplacerTest' (8 tests passed; Maven reactor succeeded) - Unit Test: ./run-fe-ut.sh --run PruneNestedColumnTest focused top-level-null and dynamic element access tests (3 tests passed; Maven reactor succeeded) - Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest (61 tests passed; Maven reactor succeeded) - Unit Test: ./run-fe-ut.sh --run 'org.apache.doris.planner.IcebergMergeSinkTest,org.apache.doris.nereids.trees.plans.commands.IcebergMergeCommandTest' (7 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: 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 - Static analysis: PATH=/mnt/disk6/common/ldb_toolchain_toucan/bin:$PATH build-support/run-clang-tidy.sh was attempted after the focused fixes. The newly added residual primitive helper no longer reports cognitive-complexity/function-size warnings, but the script still fails in this local worktree on pre-existing warnings in touched large reader functions and 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, UUID, and residual primitive VARIANT values, validates wrapper fields more strictly, avoids row-wise reconstruction for supported typed-only shredded projections, preserves residual binary/null/primitive values, typed-array null elements, nullable complex pruning, user value-only fields, field-level residual values for mixed typed/residual shredded subpaths, and complex array elements with binary leaves, preserves full VARIANT projection when predicates read typed subpaths, handles dynamic and non-slot 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, rejects Iceberg VARIANT data-file writes explicitly, and keeps delete-only Iceberg MERGE on VARIANT tables on the delete-file path. - Does this need documentation: No
|
/review |
|
run buildall |
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