branch-4.1:[feature](iceberg) Support Iceberg V3 default values - #66538
branch-4.1:[feature](iceberg) Support Iceberg V3 default values#66538hubgeter wants to merge 14 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
def80c9 to
7c6b07b
Compare
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
7c6b07b to
3046194
Compare
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
6639cf6 to
2a46b1b
Compare
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes with nine validated inline findings.
Review completion status: capped/incomplete. A new accepted issue appeared in the third and final normal review round, so the required three-round cap was reached and no fourth round was permitted. All nine accepted findings are included here.
Static checkpoints covered:
- Schema evolution, defaults, and read correctness: findings 1, 2, 7, and 8.
- Equality-delete correctness and historical typing: findings 1 and 3; position-delete routing and ownership were also traced without a separate surviving issue.
- Planner and transaction lifecycle: findings 4 and 5; pinned schema/spec/branch state, retry, and final metadata CAS otherwise remained consistent.
- ORC/Parquet interoperability: findings 6 and 9; UUID/FIXED/BINARY normalization was traced across FE and BE.
- Mixed-version execution: scan carriers and source-BE fences were reviewed; finding 9 identifies the missing write-side fence.
- Coverage: static review only, as required by the review runner; no builds or tests were run. Existing tests do not exercise the reachable cases described inline.
- User focus: no additional user-provided focus was supplied.
| column_ids.insert(column_id); | ||
| } | ||
| _all_required_col_names.push_back(block_name); | ||
| table_info_node_ptr->add_children(block_name, file_column->name, |
There was a problem hiding this comment.
[P1] Share the physical root across nested equality keys
This adds a new table alias for every nested equality key, but ParquetReader::init_reader() reverses these mappings with required_file_columns.emplace(fileName, tableName), so one physical root can populate only the first alias. If the struct is projected normally, that slot wins; if two delete keys share the struct, only one hidden alias wins. The other hidden column remains empty and equality deletes are missed (or fail on the size mismatch). Please read each physical root once and extract all nested keys from that populated column.
| RETURN_IF_ERROR(_expand_block_if_need(block)); | ||
|
|
||
| RETURN_IF_ERROR(_file_format_reader->get_next_block(block, read_rows, eof)); | ||
| RETURN_IF_ERROR(_materialize_missing_table_columns(block, *read_rows)); |
There was a problem hiding this comment.
[P1] Materialize whole-column defaults before V1 filtering
For a struct/list/map that is entirely absent from an old file, FE gives the physical reader a NULL placeholder and relies on this call to install the real Iceberg initial default. However, both V1 Parquet and ORC evaluate missing-column conjuncts inside get_next_block() before control reaches this line. Predicates such as added_struct IS NOT NULL or a nested comparison therefore discard valid rows based on NULL and cannot be repaired afterward. The real typed default must reach the physical reader, or these conjuncts must be deferred until after materialization.
| delete_col_names.push_back(leaf_name); | ||
| delete_col_types.push_back(leaf_type); | ||
| _equality_delete_col_ids.insert(field_id); | ||
| if (!_id_to_block_column_name.contains(field_id) && |
There was a problem hiding this comment.
[P1] Preserve the historical type for each equality delete
This suppresses the delete-typed hidden carrier whenever the field is already projected. After a legal INT-to-BIGINT promotion, one old INT equality-delete file is therefore probed with the current BIGINT data column: the multi-key path rejects the type mismatch, and the simple-key HybridSet hits its exact assert_cast, failing the scan. Grouping multiple delete files only by their ID vector has the same problem when it tries to merge old and new key types. Keep predicates/carriers per delete schema and cast the data key to each historical type before probing.
| return buildMergePlan(ctx, logicalQuery, assignments, icebergTable, writeSchemaContext); | ||
| } finally { | ||
| ctx.setIcebergRowIdTargetTableId(previousTargetTableId); | ||
| IcebergDmlCommandUtils.restoreWriteSchemaContext(ctx, previousWriteSchemaContext); |
There was a problem hiding this comment.
[P2] Keep the pinned context through EXPLAIN analysis
getExplainPlan() returns an un-analyzed merge tree, but this restores the Iceberg write context before ExplainCommand calls planner.plan(). Assignment/value DEFAULTs happen to be rewritten eagerly; DEFAULT(target_column) in UPDATE WHERE, MERGE ON, or action predicates reaches RewriteDefaultExpression later and fails because the pinned target is gone, although executing the same DML succeeds. Carry the context through explain planning or eagerly rewrite every DEFAULT occurrence before restoring it.
| checkFileScannerV1BackendCompatibility( | ||
| context.getSessionVariable().enableFileScannerV2, backendPolicy.getBackends()); | ||
| boolean batchMode = isBatchMode(); | ||
| boolean batchMayHaveEqualityDeletes = batchMode && mayHaveEqualityDeletes(); |
There was a problem hiding this comment.
[P1] Preserve lazy planning on current-only backends
This calls mayHaveEqualityDeletes() for every batch scan. Unless the snapshot summary proves zero, that method synchronously invokes scan.planFiles() and can exhaust the full filtered task set before super.createScanRangeLocations() starts the lazy producer, which plans the scan again. On large tables this defeats batch planning even when every selected backend supports the new semantics. Gate this exact preflight on a smooth-upgrade source backend (or otherwise avoid planning tasks twice).
| continue; | ||
| } | ||
| const auto value = column->get_data_at(row); | ||
| if (value.size != expected_length) { |
There was a problem hiding this comment.
[P1] Pad legacy CHAR values for Iceberg FIXED
With varbinary mapping disabled, Iceberg FIXED[n] is exposed as Doris CHAR(n). A short CHAR payload is normal and the Parquet fixed-size writer zero-pads it to n bytes, but this ORC path rejects every value whose stored StringRef is shorter than n because it does not receive the Doris type. Valid inserts therefore fail only when the table writes ORC. Preserve the existing TYPE_CHAR padding contract while keeping exact-length validation for binary/string carriers.
| for (NestedField field : fieldById.values()) { | ||
| NestedField historicalField = historicalFieldById.get(field.fieldId()); | ||
| if (historicalField != null) { | ||
| if (!collectionWrapperFieldIds.contains(field.fieldId()) |
There was a problem hiding this comment.
[P2] Enforce same-ID optional-to-required evolution
This gate misses two present-field transitions: collectionWrapperFieldIds contains every list element/map value stable ID, and initialDefault() != null excludes scalar or struct fields. Neither exception makes a historical explicit NULL valid: defaults fill missing fields only, and a replaced wrapper is different from a stable-ID evolution. Both readers keep these carriers nullable and consult requiredness only when a field is missing, so current backends can return NULL and source backends are left unfenced. Treat every same-ID optional-to-required transition as requiring current semantics and validate present null maps.
| dateTime.getDayOfMonth(), dateTime.getHour(), dateTime.getMinute(), | ||
| dateTime.getSecond(), microsecond); | ||
| } | ||
| return new DateTimeV2Literal((DateTimeV2Type) targetType, |
There was a problem hiding this comment.
[P1] Keep legacy timestamptz defaults as instants
With timestamp-tz mapping disabled, this turns the Iceberg default's UTC microsecond instant into a timezone-less UTC-wall DATETIMEV2. Parquet's Iceberg Arrow schema and the ORC serializer then interpret that wall time in the session timezone, so in Asia/Shanghai a 01:02Z default is persisted as 17:02Z on the prior day. Missing-field defaults have the sibling inconsistency: their offset is stripped while physical adjusted-to-UTC values decode to session-local time. The new UTC-only regression masks both paths. Convert defaults to the session-local wall time consistently, or retain an instant-aware carrier through serialization.
| const auto use_iceberg_binary_type = [&](std::string_view binary_type) { | ||
| DORIS_CHECK(is_string_type(primitive_type) || is_varbinary(primitive_type) || | ||
| primitive_type == TYPE_BINARY); | ||
| type = orc::createPrimitiveType(orc::BINARY); |
There was a problem hiding this comment.
[P1] Fence binary ORC writes from old backends
These Iceberg-specific ORC types and the UUID normalization below exist only on upgraded BEs, but FE write validation still lets a query-available smooth-upgrade-source BE execute this sink. The baseline BE receives the same Iceberg schema yet emits generic STRING/CHAR or unannotated BINARY carriers and does not convert UUID text to 16 bytes. A distributed insert can therefore commit different, non-Iceberg physical schemas depending on which BE writes each file. Add a write capability gate or scheduler restriction analogous to validateVariantWriteBackendCompatibility() for ORC schemas containing UUID/FIXED/BINARY, or provide a wire-compatible fallback.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
Issue Number: None Related PR: apache#65851 Problem Summary: Iceberg V3 separates initial defaults for fields absent from older data files from write defaults for omitted fields. Doris branch-4.1 previously returned NULL for missing fields and did not consume Iceberg write defaults consistently across schema evolution, branch writes, equality deletes, and the V1/V2 readers. This change carries typed defaults by field ID, materializes missing values in both reader generations, pins write planning and MERGE distribution to the same target schema/spec, and applies write defaults for omitted columns or explicit DEFAULT. It also keeps V1 byte equality deletes compatible across STRING/VARBINARY carriers and adds FE-to-Thrift and ORC byte-level coverage. The latest semantic rebase integrates branch-4.1's generation-aware Iceberg metadata cache by acquiring writable tables through the catalog-generation fence instead of retaining the obsolete fresh-load helper. Existing Iceberg VARIANT read/write validation and transform-aware writer distribution are preserved; VARIANT default values remain outside this backport scope. Support Iceberg V3 initial defaults for fields absent from older files and write defaults for omitted columns or explicit DEFAULT values. VARIANT default values are not included. - Test: Unit Test / Regression test / Manual test - Latest branch-4.1 rebase: full FE build, Checkstyle, 5,148 main sources, and 1,308 test sources passed via `./build.sh --fe -j 12` - Latest branch-4.1 rebase: `IcebergExternalMetaCacheTest` 76/76 and `IcebergTransactionTest` 25/25 passed - Previous candidate: full ASAN BE build/link and focused BE UT 6/6 passed; BE feature paths are unchanged by this FE-only conflict resolution - Previous candidate: `test_iceberg_write_default` passed 1/1 and generated its `.out` through the regression runner - Current rebase: `git range-diff`, conflict-marker scan, and `git diff --check` passed - Other external Iceberg regression suites were not rerun after the latest rebase - Behavior changed: Yes. Iceberg reads and writes now apply V3 initial and write defaults for supported non-VARIANT types; writable-table acquisition follows branch-4.1 catalog-generation fencing. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: The branch-4.1 Iceberg V3 default-value backport still had correctness gaps in V1 equality-delete carrier planning, pre-filter complex defaults, EXPLAIN context lifetime, batch compatibility preflight, required-field evolution, ORC FIXED encoding, timestamptz semantics, and mixed-version ORC binary writes. Preserve one shared physical root while materializing delete-schema-specific carriers, move defaults and validation to the correct execution boundary, retain pinned write metadata through planning, and add focused compatibility fences and tests. ### Release note None ### Check List (For Author) - Test: Unit Test and local ASAN BE/FE build - 175 focused FE tests and 115 focused BE tests passed - Behavior changed: Yes (fixes correctness and rolling-upgrade safety for the new Iceberg V3 default-value feature) - Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Automated review of 74aadf722b48d631bb0d0a6859b686c5bfe7321b.
Found one new blocking correctness issue: the two new recursive complex-default readers parse Iceberg DOUBLE JSON tokens with RapidJSON normal precision, so a valid initial default can change before it is materialized for historical rows.
Critical checkpoints completed:
- Traced the pinned writer generation through INSERT, OVERWRITE, UPDATE, MERGE, branches, empty results, final commit, and retries; no additional bypass found.
- Checked V1/V2 initial defaults, requiredness, equality deletes, predicate/aggregate order, and mixed-version gates; the inline DOUBLE precision issue is the only new distinct finding.
- Checked current/historical field identities and types through query-wide and split-local Thrift carriers; remaining cases are covered by existing threads.
- Checked DEFAULT parsing/binding and INSERT/UPDATE/MERGE plus EXPLAIN/PREPARE context lifecycles; no additional issue found.
- Deduplicated against all 15 existing inline threads.
No additional user-provided focus was supplied. Per the review instructions, no builds or tests were run.
| const auto primitive_type = value_type->get_primitive_type(); | ||
| if (is_complex_type(primitive_type)) { | ||
| rapidjson::Document document; | ||
| document.Parse(field.initial_default_value.data(), field.initial_default_value.size()); |
There was a problem hiding this comment.
[P1] Parse complex DOUBLE defaults without losing bits
Iceberg's single-value encoding stores DOUBLE leaves in list/map initial defaults as JSON numbers. This default Document.Parse uses RapidJSON's normal-precision conversion (documented as allowing up to 3 ULP of error), then json_scalar_text serializes the already-rounded double and from_fe_string materializes it. An older row missing a valid list<double> or map<..., double> field can therefore silently receive a different value from its declared initial default. Use kParseFullPrecisionFlag (or preserve numeric tokens) here and in the mirrored V2 parser at be/src/format_v2/table/iceberg_reader.cpp:412, and add a boundary DOUBLE regression.
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: Iceberg complex initial defaults encode DOUBLE leaves as JSON numbers. Both V1 and V2 readers used RapidJSON normal-precision parsing before serializing those values into Doris columns, which can round a valid boundary value to a different IEEE-754 bit pattern. Parse complex defaults with RapidJSON full precision in both reader generations and add a one-ULP regression that verifies the exact expected double bits. ### Release note Preserve exact DOUBLE values in Iceberg complex initial defaults. ### Check List (For Author) - Test: Unit Test - IcebergReaderTest.materializes_complex_double_initial_default_at_full_precision passed under ASAN - IcebergV2ReaderTest.AnnotateBuildsComplexDoubleDefaultAtFullPrecision passed under ASAN - clang-format, format check, and git diff check passed - Behavior changed: Yes (complex DOUBLE defaults retain their correctly rounded IEEE-754 value) - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes for two P1 correctness issues.
Critical checkpoint conclusions:
- FE DEFAULT resolution: blocking issue. Non-inline Iceberg INSERT-SELECT binds DEFAULT(column) in the source scope instead of the pinned target scope, so staging/no-FROM forms fail or can use the wrong default.
- BE V2 nested materialization/requiredness: blocking issue. Partial struct projections can materialize children in table order but validate them against raw lexical mapping order, checking the wrong sibling for NULL.
- V1/V2 scan ordering: equality deletes, defaults, requiredness, predicates, aggregates, all-filtered batches, and nullable ARRAY/MAP/STRUCT masks were traced end to end; no distinct issue remains beyond the struct-order finding or existing review threads.
- Equality-delete schema lineage: exact and lazy-batch split-local carriers, historical field identity/type, expired/cherry-picked ancestry, and rolling-upgrade fallback were checked; no new issue remains beyond existing threads.
- Write-schema pinning: INSERT/OVERWRITE/UPDATE/MERGE/EXPLAIN propagation, sink copies, writer serialization, transaction replay, empty commits, and final metadata validation were checked; no new issue found.
- Compatibility/default transport: Parquet/ORC, BINARY/FIXED/UUID, timestamp defaults, recursive complex defaults, and mixed-version gates were checked; no new issue remains beyond existing threads.
The full 103-file authoritative diff and all 16 existing review threads were reviewed and deduplicated. Round 2 converged with three independent NO_NEW_VALUABLE_FINDINGS results while independently upholding both comments below. No builds or tests were run, as required by the review prompt.
User focus: no additional focus was provided.
|
|
||
| Optional<IcebergWriteSchemaContext> icebergContext = context.cascadesContext | ||
| .getStatementContext().getIcebergWriteSchemaContext(); | ||
| Optional<TableIf> originalTable = slotRef.getOriginalTable(); |
There was a problem hiding this comment.
[P1] Bind INSERT-SELECT DEFAULT against the write target
At this point the child is already bound in the SELECT input scope. For INSERT INTO iceberg_t(id, v) SELECT id, DEFAULT(v) FROM staging_t, v either fails binding when absent or binds staging_t.v; in the latter case this falls through to the staging column's Doris default/NULL instead of the pinned Iceberg write default. The positive test only self-scans iceberg_t, which makes source and target identity coincide. Pre-resolve DEFAULT nodes for non-inline INSERT SELECT against the write-schema context, as VALUES/UPDATE/MERGE do, and cover staging-table and no-FROM SELECTs.
| const auto& struct_column = assert_cast<const ColumnStruct&>(*nested_column); | ||
| DORIS_CHECK(mapping.child_mappings.size() == struct_column.tuple_size()); | ||
| for (size_t child = 0; child < mapping.child_mappings.size(); ++child) { | ||
| RETURN_IF_ERROR(_validate_required_mapping_column(mapping.child_mappings[child], |
There was a problem hiding this comment.
[P1] Match struct validation by table child identity
_materialize_struct_mapping_column() reorders child mappings into DataTypeStruct order before building the ColumnStruct, but this loop zips that reordered tuple with raw mapping.child_mappings. Partial access paths are stored lexically, so a schema-order STRUCT<z optional, a required> can produce mappings [a,z] and columns [z,a]; requiredness for a is then checked on z, allowing NULL a through or rejecting a valid NULL z. Reorder mappings here with the same table-order helper and add an opposite-requiredness partial-projection regression, including a struct nested under ARRAY/MAP.
### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: Iceberg INSERT-SELECT DEFAULT expressions were bound against the SELECT input scope instead of the pinned write target, so a same-named staging column could replace the target write default or a no-FROM query could fail analysis. Required-field validation also zipped projected struct mappings with materialized struct columns even when partial projection reordered the mapping list. Resolve INSERT-SELECT DEFAULT references from the pinned Iceberg write schema before source binding, and validate struct children in table type order using the same ordering helper as materialization. ### Release note Fix Iceberg INSERT-SELECT default handling and nested required-field validation. ### Check List (For Author) - Test: Unit Test / focused compilation - FE IcebergDDLAndDMLPlanTest#testIcebergInsertExplicitDefaultAndSelectOmission passed (1/1) - ASAN_UT compiled the changed Iceberg reader production and unit-test translation units - clang-format, check-format, and git diff --check passed - Behavior changed: Yes (Iceberg defaults bind to the write target and nested requiredness follows table field order) - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed head cdb0fb9 against base facf9cc across the full 103-file bundle. I found no additional valuable findings beyond the existing inline threads. The Iceberg default transport/materialization, equality-delete identity and historical typing, requiredness/filter ordering, pinned write metadata, Parquet/ORC writer contracts, and mixed-ID/name-mapping paths were checked end to end; the independent BE, FE, and cross-layer passes all converged with no new findings. Per the review instructions, I did not run builds or tests.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
…quired validation ### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: Iceberg missing columns now carry a VLiteral default expression, so RowGroupReader::_fill_missing_columns takes the default-value branch and replaces the whole Block column with one sized to the current batch. The row-id fetch path builds its ParquetReader with batch_size 1 and appends every batch into a single Block, so that replacement truncated the accumulated column and tripped the block/column size check in vparquet_group_reader.cpp, aborting the BE and taking the rest of the external regression run down with it. Required-field validation also walked every tuple slot, including the slots a count(*) scan keeps but never materializes, so a column that was never read from the file reported a spurious "Required Iceberg field contains NULL". Size the replacement column by the rows it already holds plus the rows the batch produced, and validate only the columns the scan actually reads. ### Release note Fix an Iceberg BE crash when fetching rows by row id from a table with a defaulted column, and a spurious required-field error on count(*). ### Check List (For Author) - Test: Unit Test - Verified under ASAN with a temporary two-row-id read_lines case: it reproduced the exact CI abort (block rows = 2 , column rows = 1, col name = id) before the fix and passes after - Reverting the validation guard alone makes the unmaterialized-slot case fail, and reverting it with the pristine fixture makes rejects_visible_null_for_required_v1_field fail, which is why set_projected_table_field now records the column as read - 1029 parquet/iceberg/table-reader/column-mapper BE unit tests pass - clang-format and git diff --check passed - Behavior changed: Yes (row-id fetch keeps accumulated default columns intact, and count(*) no longer fails on required slots it does not materialize) - Does this need documentation: No
|
run buildall |
|
/review |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Request changes.
I found two remaining P1 correctness issues:
- Pinned-target DEFAULT pre-resolution skips several INSERT query representations, producing source-default semantics or binding failures depending on plan shape.
- V2 reconstructs an internally inconsistent missing nested equality-key path after a legal type promotion.
Coverage/checkpoints:
- Reviewed all 103 authoritative changed files and the required FileScannerV2 design/review documents.
- V1/V2 schema identity, defaults, requiredness, equality-delete ordering, nested paths, and historical types: the V2 promotion/missing-ancestor issue below remains; existing review threads were treated as hard duplicate fences.
- Writer-schema MVCC, INSERT/OVERWRITE/UPDATE/MERGE/EXPLAIN context propagation, transaction validation, and final commit checks: no additional distinct issue beyond the INSERT DEFAULT traversal below.
- Mixed-version gates, task reuse/count paths, Thrift carriers, Parquet/ORC/Arrow writers, and test/regression changes: no additional distinct issue survived verification.
- User focus: no additional focus was provided.
Convergence status is capped/incomplete: genuinely new accepted findings appeared in Round 3, and the review process is limited to three rounds. All findings discovered in those rounds are reconciled and included here.
No builds or tests were run, per the review instructions.
| Plan query, | ||
| Optional<IcebergWriteSchemaContext> writeSchemaContext, | ||
| List<String> targetNameParts) { | ||
| return query.rewriteUp(plan -> { |
There was a problem hiding this comment.
[P1] Resolve DEFAULTs in every INSERT query representation
This traversal handles only Project/one-row expressions. Grouped outputs live on LogicalAggregate/LogicalRepeat; WITH producers are extra plans attached later; scalar subqueries own a query plan inside a leaf expression; and VALUES under set operations remain an UnboundInlineTable leaf. Each shape bypasses the pinned-target rewrite, so DEFAULT(name) can use a staging default or fail binding depending on representation. Rewrite/rebuild all four output carriers with the pinned context, and cover INTO/OVERWRITE plus affected EXPLAIN forms.
| std::vector<ColumnDefinition> result; | ||
| result.reserve(external_path->size()); | ||
| for (size_t index = 0; index < external_path->size(); ++index) { | ||
| result.push_back(build_schema_column_metadata_from_external_field( |
There was a problem hiding this comment.
[P1] Keep synthetic ancestor types consistent after promotion
path_types builds the ancestors from the delete-file leaf type before this metadata restore. With an old INT delete key and current BIGINT payload.k, that yields a STRUCT<INT> ancestor but a BIGINT leaf definition. If an older data file lacks the whole defaulted struct, V2 creates the default as STRUCT<INT>, then NestedStructFieldExpr allocates BIGINT and inserts the INT child before the later cast back to the delete key, causing an invalid column-type operation. Restore the current leaf type first and build ancestors bottom-up from it (or keep the whole path consistently historical), with Parquet/ORC missing-struct plus INT-to-BIGINT coverage.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…d validation ### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: The previous commit let IcebergTableReader::_validate_required_table_columns skip any tuple slot missing from _all_required_col_names, on the theory that a `count(*)` scan keeps a required slot it never materializes. The external regression run disproved it: the four remaining `Required Iceberg field ... contains NULL` failures come from FileScannerV2, not from this V1 reader, so the exemption changed no test outcome while relaxing a contract check and forcing the test fixture to model a projected field as read. Restore the original validation and fixture; the row-id fetch column-sizing fix in vparquet_group_reader.cpp is unaffected and stays. ### Release note None ### Check List (For Author) - Test: Unit Test - IcebergReaderTest, ParquetReadLinesTest and IcebergV2ReaderTest: 116/116 pass under ASAN - 1029 parquet/iceberg/table-reader/column-mapper BE unit tests pass - clang-format and git diff --check passed - Behavior changed: Yes (V1 required-field validation returns to covering every projected slot) - Does this need documentation: No
… columns ### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: TableReader marks every non-predicate output column of a COUNT(*) scan as a count-star placeholder, so the file reader never decodes their values and the block carries placeholders that only transport the surviving row count. IcebergTableReader::materialize_virtual_columns validated every mapping with reject_null_value against that block, so `select count(*)` over a table with a required Iceberg field reported "Required Iceberg field '<name>' contains NULL" for data that has no NULL at all. The placeholder contract already states these values must not be decoded or validated; honour it in the Iceberg required-field check. ### Release note Fix a spurious "Required Iceberg field contains NULL" error on count(*) over an Iceberg table with a required field. ### Check List (For Author) - Test: Unit Test - Reproduced locally under ASAN: COUNT(*) pushdown with an empty count-column list over a required projected field returned the exact production error on a file whose values are all non-NULL; the same case now returns the rows and the count - A control case with a genuine NULL in a required field is still rejected, so the check is not weakened - 1029 parquet/iceberg/table-reader/column-mapper BE unit tests pass - clang-format and git diff --check passed - Behavior changed: Yes (COUNT(*) no longer validates columns it never decodes) - Does this need documentation: No
…deferred predicates ### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: _filter_deferred_required_column_predicates() passed cast_set<uint16_t>(block->rows()) as the selected-row count while Block::ScopedMutableColumns had already borrowed every column out of the Block. A Block with no columns reports zero rows, so the deferred predicate was always evaluated over an empty selection, returned zero surviving rows, and the subsequent all-zero filter dropped every row. Any query whose predicate on a required Iceberg field is deferred out of the physical reader therefore returned an empty result, for example `select count(*) ... where <required> >= N` returning 0. Capture the row count before entering the guard scope. ### Release note Fix Iceberg queries returning no rows when a predicate on a required field is evaluated after materialization. ### Check List (For Author) - Test: Unit Test - Reproduced locally under ASAN: a four-row non-NULL block of 4,5,6,7 filtered by `>= 6` returned 0 rows before the fix and 2 after, both in isolation and end to end through the V1 Iceberg reader over the equality_delete_par_1 fixture file - 1031 parquet/iceberg/table-reader/column-mapper BE unit tests pass - clang-format and git diff --check passed - Behavior changed: Yes (deferred required-field predicates now evaluate over the real rows) - Does this need documentation: No
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…nd fix the ORC default fill ### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: This backport added a runtime required-field check that upstream does not have: ColumnMapping::reject_null_value, IcebergTableReader::_validate_required_mapping_column, _validate_required_table_columns, and the predicate deferral built to feed them (_required_validation_slot_ids, _deferred_required_column_predicates). apache/master keeps only the mapping-time contract, reject_missing_required_field, which rejects a required field that is absent from the data file. The runtime scan repeatedly reported "Required Iceberg field ... contains NULL" for columns a COUNT(*) plan never decodes, because such a plan keeps its projected columns purely as row-count placeholders. Remove that machinery and keep the mapping-time check, matching upstream. Separately, OrcReader::_fill_missing_columns had the same batch-sizing bug already fixed for Parquet: the row-id fetch path appends several batches into one Block, so replacing the column with one sized to the current batch truncated it and aborted the BE. ### Release note Fix spurious "Required Iceberg field contains NULL" errors and an Iceberg BE crash when fetching rows by row id from an ORC table with a defaulted column. ### Check List (For Author) - Test: Unit Test - Full BE unit test run under ASAN: 10435 tests, identical failure set to the pre-change baseline (25, all missing local test data: JsonReader, apache-orc examples, shredded variant) - 1022 parquet/iceberg/table-reader/column-mapper tests pass - Verified locally that COUNT(*) over a required Iceberg field now returns its rows instead of the spurious error, and that a required-field predicate no longer empties the Block - clang-format and git diff --check passed - Behavior changed: Yes (a visible NULL in a required Iceberg field is no longer rejected at read time, matching apache/master) - Does this need documentation: No
|
run buildall |
3 participants
What problem does this PR solve?
Problem Summary:
Backport #65851
Support Iceberg V3 default values
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)