Repository navigation
[fix](be) Compare Iceberg equality-delete keys in the wider of the delete and data file types - #69711
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
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.
Three P2 regressions remain in the equality-delete comparison change: imported INT96 timestamp keys can be rejected against normal timestamp tombstones, a missing struct default can cause a provably NULL historical key to be rejected, and an empty nanosecond delete set can cause a timestamp overflow. All three are reported inline; no existing inline findings were present to carry forward.
Reviewed all five changed files and the complete affected path: scan/split preparation -> delete-file schema and loading -> field-ID/name/history resolution -> nested extraction/defaults -> per-file comparison selection -> cached converted block/hash index -> file-local predicate -> table materialization. The comparison change belongs to table semantics and continues to use format-neutral schemas and expressions. External compatibility was checked against the Iceberg specification and reference implementation, including imported representations.
Critical checkpoint conclusions:
- Goal and proof: per-delete/data-file widening addresses the reported scalar histories without narrowing keys. Nine new BE tests cover those histories, but the three inline cases remain uncovered and prevent considering the fix complete.
- Scope and reuse: the four production-file changes are focused; conversion reuses EvolutionCast, existing predicates and the split cache. No unrelated source changes.
- Concurrency and locks: the cache is shared by scanners of one scan node. Existing per-entry construction locks protect immutable results; conversion adds no lock inversion or shard-wide heavy-work lock.
- Lifecycle and memory safety: converted predicates retain owning column pointers and a shared immutable index. Const block execution preserves original cached columns; expression contexts close through RAII, failed factories are not admitted, and split-local state resets. No new cross-translation-unit static initialization.
- Configuration: no new option or dynamic-configuration behavior.
- Compatibility: no storage, wire, serialized-block or dependency/linker format changes. External physical representations and historical defaults need the fixes identified inline; final binary ABI was not exercised.
- Thrift/protobuf IDs: no gensrc changes, so cross-branch field-ID comparison is inapplicable.
- Parallel paths: V1 remains intentionally unchanged. Other EvolutionCast users retain strict=false by default, and strictness survives cloning. Parquet/ORC and existing UUID/binary/name-mapping paths were traced.
- Conditional branches and errors: new branches have concrete explanatory examples, and conversion/type errors propagate through Status. The missing-root default condition is semantically incomplete as described inline, and empty delete sets must be removed before comparison casts. Strict out-of-range timestamp conversion for nonempty sets deliberately fails rather than creating a NULL key.
- Test coverage: inspected all nine additions and adjacent equality-delete, nested-default, cache and cast tests. Alternating INT/LONG splits exercise reuse; overflow and incompatible types have negative assertions. Existing composite matching supports the underlying predicate, but composite widening, conversion contention and explicit strict=true clone combinations lack focused new tests. Empty tombstone coverage is a concrete correctness gap identified inline.
- Expected results: survivor vectors and error assertions match the scalar inputs on inspection; no regression .out files changed. No tests were executed.
- Observability: debug output now distinguishes table and delete key types, existing error text identifies the key/data file, and newly cached indexes update the hash-index memory counter. No additional critical metric requirement was identified.
- Transactions and persistence: this is a read-side change; snapshot/sequence applicability is supplied by existing planning. No EditLog, failover, MoW bitmap or persistence change.
- Data writes and atomicity: no write or commit path changed.
- FE/BE variable propagation: no new transmitted variable; the FE carrier was inspected to verify historical types and defaults.
- Performance: unchanged keys share columns; conversion/index construction occurs once per cached comparison-type tuple rather than per row or block. No benchmark was run, so performance is assessed structurally.
- Additional correctness: checked null masking, ancestor NULL propagation, decimal widths/scales, exact integer widening, timestamp range errors, binary hashing and cache identity. No further verified regression was found in the completed scope.
There was no additional user-provided focus. Review evidence is static only: no builds, tests, benchmarks, runtime queries, external-writer execution, source edits or submodule initialization. The new fixtures are synthesized Parquet; direct ORC widening and external Spark/Hive interoperability execution remain validation gaps. These are testing limits, not unresolved review candidates.
Review converged after 3 round(s).
| to_type->get_precision() >= from_type->get_precision(); | ||
| case TYPE_DATEV2: | ||
| return to_primitive == TYPE_DATETIMEV2 || to_primitive == TYPE_TIMESTAMP_NS; | ||
| case TYPE_DATETIMEV2: |
There was a problem hiding this comment.
[P2] Resolve INT96 timestamp semantics before selecting the key type
With enable_mapping_timestamp_tz=true, an imported Spark/Hive Parquet file can expose an Iceberg timestamptz key as DATETIMEV2(6) (INT96), while a normal adjusted-UTC INT64 equality-delete file exposes the same logical key as TIMESTAMPTZ(6). Iceberg's importer explicitly treats INT96 as a zoned timestamp. For an id-only query the hidden key reaches this selector from the raw file schema, before ParquetReader::open applies projection timestamp semantics. Neither widening direction accepts this pair, so the scan now fails with NotSupported, even in UTC; previously both keys were converted to the table's TIMESTAMPTZ type and the delete applied. Resolve the carrier's timestamp semantics from the Iceberg field metadata before choosing the comparison domain, and cover imported INT96 data against ordinary timestamp tombstones.
There was a problem hiding this comment.
Confirmed, and fixed in 3e250364574. Parquet INT96 carries no instant annotation, so the V2 Parquet schema maps it to DATETIMEV2(6) (NativeFieldDescriptor::get_doris_type), while an adjusted-UTC INT64 delete key maps to TIMESTAMPTZ(6); neither direction of the new widening table accepts that pair, so the split failed with NotSupported where the previous code compared both sides in the table's TIMESTAMPTZ. One correction: this is not limited to id-only queries. Nothing re-types an INT96 column of an Iceberg scan at open: apply_projection_timestamp_semantics acts only on timestamp_is_adjusted_to_utc, which only the Paimon connector sets, and the key is resolved from the reader's file schema in every case, so SELECT ts failed the same way. enable_mapping_timestamp_tz is always true on master, so this was the default configuration. The fix compares a DATETIMEV2 / TIMESTAMPTZ pair in TIMESTAMPTZ, at the larger of the two scales, when the table field is timestamptz, converting the DATETIMEV2 side with the same session-time-zone cast the projection applies to that column (strict, like the other comparison casts), so a tombstone matches exactly the value a query returns. IcebergEqualityDeleteComparesInt96DataKeyAsTimestampTz reads an INT96 data file against an adjusted-UTC tombstone in Asia/Shanghai, with and without the key projected.
| auto cast_expr = EvolutionCast::create_shared(filter.key_types[key_idx]); | ||
| cast_expr->add_child(default_expr); | ||
| default_expr = std::move(cast_expr); | ||
| if (missing_root.initial_default_value.has_value()) { |
There was a problem hiding this comment.
[P2] Recognize NULL leaf defaults inside a struct default
A non-null default on missing_root does not mean the extracted key is non-null: Iceberg permits a struct default {} whose child defaults to NULL. Consider a missing payload struct containing only optional k UNKNOWN, with parent default {} and child default NULL. Promote k to DATE without a snapshot, prepare a NULL DATE equality delete, drop payload, commit the delete, then clean expired metadata while retaining the UNKNOWN snapshot. Only the historical UNKNOWN definition remains, so FE sends k as STRING. The recursive default builder correctly produces a NULL STRING leaf, but this branch sees the parent's {} and rejects DATE/STRING before NULL matching. Previously the NULL tombstone and default matched in STRING. Determine whether the extracted leaf is provably NULL, including ancestor defaults, and use the tombstone domain for that case; extend the missing-whole-struct test with this history.
There was a problem hiding this comment.
Confirmed in the reader, and fixed in 3e250364574. Under a struct initial default {}, a sub-field without its own default extracts as NULL, so the key should be compared in the delete file's type exactly like a missing root without a default; the branch now checks the extracted leaf instead of the root, walking the default value along the key's path with the same child matching the key expression uses. On reachability: this history cannot reach the reader through the current FE. Iceberg 1.12, which the FE uses, rejects any non-null default on a nested type (Types.NestedField.castDefault: "Invalid default value for struct ... (must be null)"), and SchemaParser cannot load "initial-default": {} on a struct (Expressions.lit has no struct literal), so such a table fails to load before planning and the FE never sends a struct initial_default_value. The BE does implement the spec's {} default (IcebergMissingWholeStructEqualityKeyMaterializesDefault), so the null-leaf case is now covered by IcebergMissingWholeStructEqualityKeyComparesNullLeafInTombstoneType.
| RETURN_IF_ERROR(equality_delete_comparison_type( | ||
| filter.field_ids[idx], filter.delete_key_types[idx], key_expr->data_type(), | ||
| _data_file_path(), &comparison_type)); | ||
| key_expr = cast_equality_delete_key(std::move(key_expr), comparison_type); |
There was a problem hiding this comment.
[P2] Skip empty delete sets before constructing strict key casts
This adds a strict comparison cast even when the loaded delete block has zero rows. Iceberg accepts zero-record equality-delete descriptors, and absent optional statistics let them reach this reader. For example, retain DATE key 1000-01-01, promote it to TIMESTAMP_NS without a snapshot, prepare an empty NS delete, then drop the key, commit the delete and clean the unreferenced promotion schema. The recovered table key is DATE. With an unrelated nonempty, nonmatching delete keeping the scan's equality-delete summary positive, the empty NS filter still reaches this line and casts the DATE key to NS. EqualityDeletePredicate evaluates its children before checking the empty hash index, so the scan fails with timestamp overflow despite no tombstone to compare. Previously the data key stayed DATE and the row survived. Skip empty loaded filters before key resolution/comparison casts, and cover this case separately from the intentional nonempty NULL-tombstone overflow error.
There was a problem hiding this comment.
Confirmed, and fixed in 3e250364574: _append_equality_delete_predicates now skips a loaded filter whose delete block has no rows before resolving its keys, so an empty delete file adds no hidden key column, no comparison cast and no predicate. Two notes on scope. Iceberg's own writers never commit such a file: the rolling writers delete a file that received no rows (BaseTaskWriter, RollingFileWriter), and with column statistics present DeleteFileIndex prunes a zero-record equality delete against data files without NULL keys, so this needs another writer that commits an empty file without statistics, which FileMetadata allows. And the evaluation order predates this PR: EqualityDeletePredicate has always evaluated its key expressions before checking for an empty index, and on master a key whose table type is TIMESTAMP_NS already failed this way on an empty delete file through the DATE -> TIMESTAMP_NS evolution cast, which is strict in every session. The skip covers both. With a nonempty delete file, a key outside the TIMESTAMP_NS range still fails as intended. IcebergEqualityDeleteSkipsEmptyDeleteFileBeforeKeyCasts covers a present and a defaulted DATE key against an empty nanosecond delete file.
TPC-H: Total hot run time: 28810 ms |
TPC-DS: Total hot run time: 151519 ms |
ClickBench: Total hot run time: 24.35 s |
…he tombstone type, and skip empty delete files ### What problem does this PR solve? Issue Number: None Related PR: apache#69711 Problem Summary: Three cases of the wider-key-type comparison of this PR: 1. Parquet INT96 carries no instant annotation, so a timestamptz key in a Spark or Hive file that Iceberg imported reads as DATETIMEV2(6), while an adjusted-UTC tombstone of the same field, as a Flink upsert writes it, reads as TIMESTAMPTZ(6). Neither widens to the other, so the scan failed with NotSupported, in any query and with the default configuration. When the table field is timestamptz, such a pair now compares as TIMESTAMPTZ at the larger scale; the DATETIMEV2 side is converted with the session time zone, as the projection converts that column, so a tombstone matches the value a query returns. 2. A missing struct whose initial default is `{}` takes each child's own default, so a child without one extracts as NULL. The comparison looked only at the root's default and rejected types that do not convert (an `unknown` key read as STRING against DATE tombstones) before NULL matching. The default is now walked down the key's path, with the key expression's field-id-else-name matching, and a NULL leaf is compared in the delete file's type. 3. A delete file without rows deletes nothing, but EqualityDeletePredicate evaluates its keys before it looks at the delete set, so the strict comparison casts still ran: a DATE key 1000-01-01 against an empty TIMESTAMP_NS delete file failed the scan with an overflow. Such a filter is now skipped before its keys are resolved; it still disables aggregate pushdown as before. ### Release note None ### Check List (For Author) - Test: Unit Test - New: `IcebergEqualityDeleteComparesInt96DataKeyAsTimestampTz` (INT96 data against an adjusted-UTC tombstone in Asia/Shanghai, with and without the key projected), `IcebergMissingWholeStructEqualityKeyComparesNullLeafInTombstoneType` and `IcebergEqualityDeleteSkipsEmptyDeleteFileBeforeKeyCasts`. - `IcebergV2ReaderTest.*:*EqualityDelete*:*ColumnMapper*:*EvolutionCast*:*TableReader*` on macOS arm64 (ASAN): 524 of 524 pass. Removing the INT96 rule, the NULL-leaf walk or the empty-file skip fails its new test. - clang-tidy (llvm 22) on the changed lines: all checks pass. - Behavior changed: No (only cases this PR made fail are affected) - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the first review round in
BE unit tests on macOS arm64 (ASAN), |
|
run buildall |
|
/review |
There was a problem hiding this comment.
No new actionable findings at head 3e250364574ea91a891afeb4eb5f25ccbfad2ee7. Review converged in 1 round: both complete-review agents and the separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS; all candidates and risk focuses were independently verified and disposed.
Existing review context: the reported INT96 mixed-carrier selector rejection (4395446119), NULL leaf under a struct default (4395446127), and empty-delete strict-cast failure (4395446137) are addressed for their originally reported cases. No duplicates are posted. There are no existing P0/P1 findings to carry forward. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and evidence: equality-delete keys retain their file types and compare in an exact common domain per data file. Inspected tests cover stale/unknown table metadata, widening, nanosecond distinctions, defaults, overflow and empty deletes.
- Scope and abstraction: the five-file change stays in table-level delete semantics with an opt-in EvolutionCast flag; physical decoding remains behind the existing reader contract.
- Concurrency and locks: the operator-shared cache serializes construction per typed key and publishes immutable completed blocks/indexes. No new mutation race or lock-order cycle was found.
- Lifecycle and memory: predicate ColumnPtrs and indexes own their data; the const execute overload leaves source Block references valid. Context cleanup, failed factories, split resets and task memory attachment were traced. No new ownership cycle or static-initialization dependency was found.
- Configuration: none added.
- Compatibility: no persisted-format or wire-layout change. Existing EvolutionCast callers retain their default behavior; cloning preserves strict mode. Unsupported comparisons and range failures propagate errors.
- Thrift/protobuf IDs: untouched.
- Parallel paths: native Parquet and ORC consume the same table-layer comparison logic; data keys and converted tombstones use the same cast helper. V1 is outside this targeted change.
- Conditional checks: widening, NULL-default and empty-filter branches document concrete cases; the empty-filter skip precedes key resolution and casts.
- Test coverage: all added tests and changed helpers were inspected, including alternating split types and cached conversion reuse. See integration limits below.
- Test results: expected survivors and errors agree with the constructed fixtures. No regression output files changed. No builds, tests, benchmarks or runtime checks were executed in this review.
- Observability: debug output includes table/delete key types; errors identify field/type/path as applicable, and converted index allocations update the existing memory counter.
- Transactions and persistence: no transaction, EditLog, version or delete-bitmap persistence path changes.
- Writes and crash atomicity: no data-write path changes.
- FE/BE variables: no transmitted fields added; existing field IDs, schema metadata and defaults were traced through their consumers.
- Performance: identical domains avoid conversion; widened tombstones and indexes are cached by source identity and ordered comparison types. No verified new per-row allocation or redundant deep-copy regression was found.
- Other issues: no additional substantiated PR regression remains after the final changed-file, candidate and risk sweep.
Coverage limits: static inspection does not establish runtime correctness. The new fixtures use synthetic scan metadata and Arrow-produced Parquet; they do not provide end-to-end FE history-cleanup or representative Spark/Flink/ORC interoperability validation. The INT96 addition exercises a legacy timestamp descriptor, leaving current timestamp-contract, reverse-tombstone and DST integration coverage absent. Dedicated concurrent/multi-key conversion and decimal/time-scale fixtures are also absent. Adjacent pre-existing timestamp behavior and unsupported default histories were distinguished from new PR defects in the ledger; these test limits are not unresolved new findings.
Review converged after 1 round(s).
…lete and data file types ### What problem does this PR solve? Issue Number: None Related PR: apache#69536, apache#68267 Problem Summary: **In short.** FileScannerV2 now compares an Iceberg equality-delete key in the wider of the delete file's and the data file's own key types, instead of in the one table type the FE sends for the whole scan. For some table histories that single type is wrong: the reader then deletes rows it must keep, keeps rows it must delete, or fails the scan. This follows the review of apache#69536, where these cases came up. **Background.** An equality delete removes a data row whose key values equal a tombstone's. After a type promotion the two sides can be stored in different types: tombstones written before an INT -> LONG promotion hold INT values that must still match LONG data. apache#68267 made FileScannerV2 compare in the current schema's type, and for a key that the current schema no longer has, the FE sends a type recovered from the retained schemas (apache#69536). The reader casts both the tombstones and the data keys to that one type. **The problem, and what it cost.** One type per scan cannot describe every delete file: | History | Table type from the FE | Delete file | Data file | Before | After | |---|---|---|---|---|---| | `expireSnapshots().cleanExpiredMetadata(true)` removed the schema of a schema-only INT -> LONG promotion of a dropped key | INT | LONG 4294967296 | INT, one NULL key | the tombstone becomes NULL in INT and deletes the NULL-key row | row kept | | A read pinned before a later DATE -> TIMESTAMP_NS promotion of the key | TIMESTAMP_NS | DATE 1000-01-01 | DATE 1000-01-01 | scan fails: `TIMESTAMP_NS overflow when casting row ...` | row deleted | | No retained schema gives the key a concrete type, so the FE sends Iceberg `unknown` as STRING | STRING | TIMESTAMP_NS 2024-01-02 00:00 | DATE 2024-01-02 | `2024-01-02` != `2024-01-02 00:00:00.000000000`, the row stays visible | row deleted | | The delete was prepared under a TIMESTAMP_NS schema that the table later abandoned for TIMESTAMP | TIMESTAMP | TIMESTAMP_NS 12:00:00.000000500 | TIMESTAMP 12:00:00.000000 | the tombstone is rounded to microseconds and deletes the row | row kept | Three of the four return wrong rows without any error. The V1 reader is not changed by this PR. **How this PR fixes it.** - The delete block keeps the delete file's own key types. When the predicate for a data file is built, each key is compared in the wider of the delete file's type and the data file's type (`equality_delete_comparison_type`). The accepted widenings are the Iceberg promotions (INT -> LONG, FLOAT -> DOUBLE, decimal precision, DATE -> TIMESTAMP and DATE -> TIMESTAMP_NS), a wider fractional scale of the same date-time kind, wider integers, STRING -> VARBINARY (an Iceberg BINARY key decodes as either one, depending on `enable_mapping_varbinary`), and TIMESTAMP -> TIMESTAMP_NS. The last one is not an Iceberg promotion; it is accepted for the abandoned-schema case above, because scaling microseconds to nanoseconds is exact. Two types that convert in neither direction fail the scan instead of being compared in a third type. - The data keys are cast to that type. When it is the data file's wider type, the tombstones are widened once per scanner, and the converted delete block and its hash index are cached next to the original (`_convert_equality_delete_filter`). A table without type changes is compared as before, with no cast and no copy. - A key that the data file lacks is compared through its initial default, in the wider of the delete file's type and the table type. Without an initial default the key is NULL in that file whatever its table type, so it is compared in the delete file's type (an `unknown` key arrives as STRING). - The comparison casts are strict whatever the session's cast mode, through a new `strict` flag on `EvolutionCast`: a non-strict overflow would turn a key into NULL and let a NULL tombstone delete it. DATE -> TIMESTAMP_NS was already strict; other `EvolutionCast` users are unchanged. - The table types from the FE still resolve table fields and the defaults of missing keys; the delete-file cache key is unchanged. **Results.** - 9 new BE unit tests in `iceberg_reader_test.cpp`: the four histories above; INT tombstones against LONG, INT and LONG data files of one scan (the third split reuses the cached widened copy); a missing key's INT default against a LONG tombstone; a missing key without a default under an `unknown` table type; a microsecond key outside the nanosecond range, which must fail; and incomparable key types. With the reader before this PR, 6 of them fail; the other three pin behavior the old code shares. Removing the TIMESTAMP -> TIMESTAMP_NS widening fails 2, and making the casts non-strict fails 1. - End to end on a local cluster: a v2 table in a `file://` Hadoop catalog, built with the Iceberg 1.12.0 Java API: `k INT` with rows (1, NULL), (2, 0), (3, 1); `k` promoted to LONG without a snapshot; a delete of `k = 4294967296` written under that schema; `k` dropped; the delete committed; then `expireSnapshots().retainLast(2).cleanExpiredMetadata(true)`, which leaves schema 0 (`k INT`) and schema 2 (no `k`). `SELECT id` through FileScannerV2 returned 2, 3 before this PR and returns 1, 2, 3 with it. branch-4.1 and branch-4.2 have the same FileScannerV2 code. ### Release note Fix FileScannerV2 comparing Iceberg equality-delete keys in a single scan-wide type, which could delete or keep the wrong rows, or fail the scan, after a key's type changed (for example a schema-only promotion removed by `cleanExpiredMetadata`, or a key whose retained schemas only have it as `unknown`). ### Check List (For Author) - Test - [ ] Regression test - [x] Unit Test - `run-be-ut.sh --run --filter='IcebergV2ReaderTest.*:*EqualityDelete*:*ColumnMapper*:*EvolutionCast*'`: 273 of 273 pass on macOS arm64 (ASAN), including the 9 new cases. - [x] Manual test (add detailed scripts or steps below) - The local end-to-end check described under Results, with an ASAN BE built from this change. - [ ] No need to test or manual test. Explain why: - Behavior changed: - [ ] No. - [x] Yes. FileScannerV2 compares equality-delete keys per delete file and data file; two key types that convert in neither direction now fail the scan; the comparison casts are strict. - Does this need documentation? - [x] No. - [ ] Yes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…he tombstone type, and skip empty delete files ### What problem does this PR solve? Issue Number: None Related PR: apache#69711 Problem Summary: Three cases of the wider-key-type comparison of this PR: 1. Parquet INT96 carries no instant annotation, so a timestamptz key in a Spark or Hive file that Iceberg imported reads as DATETIMEV2(6), while an adjusted-UTC tombstone of the same field, as a Flink upsert writes it, reads as TIMESTAMPTZ(6). Neither widens to the other, so the scan failed with NotSupported, in any query and with the default configuration. When the table field is timestamptz, such a pair now compares as TIMESTAMPTZ at the larger scale; the DATETIMEV2 side is converted with the session time zone, as the projection converts that column, so a tombstone matches the value a query returns. 2. A missing struct whose initial default is `{}` takes each child's own default, so a child without one extracts as NULL. The comparison looked only at the root's default and rejected types that do not convert (an `unknown` key read as STRING against DATE tombstones) before NULL matching. The default is now walked down the key's path, with the key expression's field-id-else-name matching, and a NULL leaf is compared in the delete file's type. 3. A delete file without rows deletes nothing, but EqualityDeletePredicate evaluates its keys before it looks at the delete set, so the strict comparison casts still ran: a DATE key 1000-01-01 against an empty TIMESTAMP_NS delete file failed the scan with an overflow. Such a filter is now skipped before its keys are resolved; it still disables aggregate pushdown as before. ### Release note None ### Check List (For Author) - Test: Unit Test - New: `IcebergEqualityDeleteComparesInt96DataKeyAsTimestampTz` (INT96 data against an adjusted-UTC tombstone in Asia/Shanghai, with and without the key projected), `IcebergMissingWholeStructEqualityKeyComparesNullLeafInTombstoneType` and `IcebergEqualityDeleteSkipsEmptyDeleteFileBeforeKeyCasts`. - `IcebergV2ReaderTest.*:*EqualityDelete*:*ColumnMapper*:*EvolutionCast*:*TableReader*` on macOS arm64 (ASAN): 524 of 524 pass. Removing the INT96 rule, the NULL-leaf walk or the empty-file skip fails its new test. - clang-tidy (llvm 22) on the changed lines: all checks pass. - Behavior changed: No (only cases this PR made fail are affected) - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3e25036 to
c313a4f
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Two new P2 findings: mixed ORC/Parquet TIME carriers reject a valid hidden equality key, and missing primitive defaults carried as STRING are rejected before a NULL physical parent can make the key NULL. The latter is verified for TIME and zoned nanosecond timestamps with all-Parquet files. Both turn previously successful id-only NULL-delete scans into errors. The inline comments describe the separate paths and fix requirements. No new P0/P1 finding was verified.
Reviewed all five changed files at c313a4f, using the supplied aggregate diff from merge base 43f9ceb. The complete path checked was FE equality schema metadata -> per-split delete-file load/cache -> physical key binding and request localization -> comparison casts/hash matching -> table materialization, including Parquet, ORC, missing/nested fields, legacy/name-based descriptors, and split reuse.
Critical checkpoint conclusions:
- Goal and correctness: the per-file domain choice addresses the documented lossy narrowing and timestamp-precision histories. The two reported physical-carrier/default rejections are the verified compatibility exceptions. Snapshot selection, equality-field identities and position-delete/DV row coordinates are unchanged.
- Scope and clarity: production changes stay within the equality-delete reader and an opt-in EvolutionCast strict flag, with focused tests and scenario comments.
- Concurrency and locks: scanners share immutable cached filters; the existing cache map/per-entry locks serialize construction. Converted-filter construction does not recursively acquire the cache. No new thread or lock order was introduced.
- Lifecycle and memory: predicates retain shared column/index owners across local filter/context destruction; const-Block execution preserves input positions. Prepared/opened local contexts close through RAII, and split preparation clears local filters. No new cross-translation-unit initializer or ownership cycle was found.
- Configuration: no configuration item was added; strict comparison casts explicitly override the existing session cast mode.
- Compatibility: no new storage or wire format is introduced. Existing descriptors still use the existing schema/name fallback. Physical-format interoperability is the source of the reported P2; runtime ABI/glibc compatibility was not measured in this static review.
- Thrift/protobuf: neither gensrc/thrift nor gensrc/proto changes.
- Parallel paths: V1 remains unchanged as scoped; other EvolutionCast callers, including Paimon precision truncation, keep their default behavior. Parquet and ORC paths were both inspected.
- Conditional checks: empty-delete, default-leaf and timestamp branches have concrete explanations; the empty-set skip precedes key resolution and strict casts. Error Status values propagate instead of silently continuing.
- Test coverage: all 12 new BE tests and fixture changes were inspected, covering type histories, split-cache reuse, missing/default/NULL keys, timestamp precision, overflow, incomparable types and INT96 hidden/projected keys. Mixed ORC/Parquet TIME and missing nested TIME/zoned-nanosecond-default/NULL-parent coverage are absent. No builds, tests or benchmarks were run for this review.
- Expected results: no regression .out file changed; the new unit-test expected IDs and negative Status assertions agree with the inspected paths. This is a static conclusion, not an executed test result.
- Observability: debug output now shows delete-file key types; existing setup timers and hash-index memory accounting cover the added setup work. No new critical logging requirement was identified.
- Transactions and persistence: no EditLog, transaction state, failover, or persistence path changes.
- Data writes and atomicity: no data-write or commit path changes; this PR affects read-side delete application.
- FE-to-BE variables: no new transmitted variable; FE schema serialization and hidden-key reachability were inspected as compatibility inputs.
- Performance: unchanged keys share columns, and widened tombstone blocks/indexes are cached per source/type tuple outside row loops. Each variant retains additional memory until operator teardown. No measured performance claim is made.
- Other concerns: namespace/local-position reconciliation, nested ancestor NULL masking, cache failure paths, strict-mode cloning and condition-cache bypass were checked; no additional verified defect was found.
Existing review context was revalidated and deduplicated. The current head skips empty delete sets before key work (4395446137), checks the extracted default leaf (4395446127), and handles the reported mixed INT96/TIMESTAMPTZ case when table metadata identifies timestamptz (4395446119). Their targeted tests are present. All existing original findings are P2; there are no existing P0/P1 comment IDs to carry. Additional variants of those same concerns were not reposted.
User focus: no additional focus was supplied. Validation limits: static source review only, without external-writer runtime interoperability, concurrent converted-cache stress, cancellation, or memory/performance measurements. The synthetic Parquet tests do not establish those broader properties or cover the reported missing-default cases.
Convergence: false after three rounds. The final sweep verified the zoned-nanosecond extension of the missing-default defect, and the final comments separate that default path from the physical TIME-carrier path. Earlier final-round payload audits covered the preceding single-comment payload; this final two-comment set has not had a further complete no-new-findings round. No unresolved concrete candidate is being withheld. The three-round cap precludes another round; both verified P2 findings are delivered.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
Three review rounds completed. The final sweep verified a zoned-nanosecond instance of the missing-default/NULL-parent defect and required splitting the prior consolidated TIME comment into two findings with separate anchors and fix scopes. The latest complete payload audits cover the preceding single-comment payload, so the final two-comment payload has not received a complete no-new-findings round from all reviewers. No unresolved concrete candidate remains; final payload-wide revalidation is the remaining coverage limit. Per the three-round cap, deliver both verified P2 findings without a fourth round.
| to_primitive == TYPE_TIMESTAMP_NS; | ||
| case TYPE_TIMESTAMPTZ: | ||
| case TYPE_TIMEV2: | ||
| return to_primitive == from_primitive && to_type->get_scale() >= from_type->get_scale(); |
There was a problem hiding this comment.
[P2] Normalize ORC TIME keys before choosing the comparison domain
An id-only scan now fails for valid mixed-format equality deletes on Iceberg TIME. The Iceberg mappings store ORC TIME as LONG with iceberg.long-type=TIME, which OrcReader exposes as BIGINT, while a Parquet TIME(MICROS, false) tombstone exposes TIMEV2(6). Neither widening direction accepts this pair. For a data row (id=1, k=NULL) and a nonempty NULL tombstone on k, FE permits SELECT id and carries the hidden key as STRING; previously both nullable casts preserved NULL and the row was deleted, but this now returns NotSupported before matching. TIME being unprojectable does not prevent this hidden dependency. Normalize its physical carriers with the correct microsecond units and add mixed ORC/Parquet hidden-key coverage; an ordinary BIGINT-to-TIME SQL cast has different semantics.
There was a problem hiding this comment.
Confirmed, and fixed in 3c2a8e9cb87. ORC stores an Iceberg TIME as a LONG annotated iceberg.long-type=TIME, which the V2 ORC reader exposes as BIGINT, while Parquet's TIME(MICROS, isAdjustedToUTC=false) reads as TIMEV2(6); the FE carries the field, which Doris cannot represent, as a STRING placeholder, so neither widening direction applied and the split failed with NotSupported. Two corrections to the framing. It was not limited to id-only scans: the column cannot be projected at all, so the key is always hidden and every scan of such a data file failed. And the previous result only held for NULL tombstones: the old code compared the pair as two differently formatted strings (3600000001 from the ORC key, 01:00:00.000001 from the Parquet tombstone), so a non-NULL tombstone on a mixed-format TIME key was silently not applied.
When the table field is that STRING placeholder and the delete and data files hold the key as BIGINT and TIMEV2, in either order, the key now compares as TIMEV2(6). The BIGINT side is converted as microseconds from midnight by a dedicated expression (IcebergTimeMicrosExpr; TIMEV2 holds microseconds, so the conversion is exact), not by CAST(BIGINT AS TIME), which reads the integer's digits as HHMMSS, and a value outside 00:00:00..23:59:59.999999 fails the scan like the other strict comparison casts. The same conversion widens ORC tombstones against a Parquet data file. Every other pair still fails as before. IcebergEqualityDeleteComparesTimeKeysAcrossOrcAndParquet covers all four ORC / Parquet data and delete file pairs with a NULL and a non-NULL tombstone; removing the rule fails the mixed pairs with NotSupported, and a SQL cast instead of the exact conversion fails them as well.
| *comparison_type = remove_nullable(filter.delete_key_types[key_idx]); | ||
| } else { | ||
| RETURN_IF_ERROR(equality_delete_comparison_type( | ||
| filter.field_ids[key_idx], filter.delete_key_types[key_idx], table_leaf->type, |
There was a problem hiding this comment.
[P2] Preserve NULL parents when resolving a missing key's default domain
This rejects valid missing-child keys before their runtime ancestor NULL mask can apply. Write Parquet data with nullable payload and a sibling field, then add optional payload.k TIME with initial default midnight (or timestamptz_ns with initial default epoch), and apply a nonempty Parquet NULL tombstone on k to a row whose payload is NULL. FE carries these unprojectable leaf types/defaults as STRING; the tombstone reads as TIMEV2 or TIMESTAMPTZ. Because the default literal is non-NULL, this call rejects STRING versus the physical type before building AncestorNullDefaultExpr, so SELECT id fails. Previously the STRING default was masked to NULL by the parent and matched the NULL tombstone. These are valid primitive defaults, and missing old-child statistics retain the delete. Resolve the typed default without rejecting this NULL-parent path, and add an all-Parquet regression; normalizing ORC TIME alone does not fix it.
There was a problem hiding this comment.
Confirmed, and fixed in 3c2a8e9cb87. For a primitive Doris cannot represent (Iceberg TIME, timestamptz_ns) the FE sends a STRING placeholder whose initial default is Iceberg's text of the value (00:00 for a TIME midnight, 1970-01-01 00:00:00+00:00 for a timestamptz_ns epoch), so a non-NULL default compared STRING against the tombstones' physical type and the split failed before AncestorNullDefaultExpr was built. One correction: this was not limited to rows under a NULL parent, since the comparison type is chosen per data file, so every data file written before the field was added failed. And the previous result only held for NULL tombstones: the default 00:00 never matched a tombstone cast to 00:00:00.000000, so a delete of the default value was silently not applied.
A non-NULL default behind such a placeholder is now parsed in the delete file's type with the strict comparison cast: TIMEV2 or TIMESTAMPTZ, and TIMEV2(6) against an ORC TIME tombstone (BIGINT, see the other thread). Rows under a NULL parent still get NULL from AncestorNullDefaultExpr and the others the parsed value; text that does not parse fails the scan. Testing this also exposed that AncestorNullDefaultExpr::open() never opened its children (its RETURN_IF_ERROR_OR_PREPARED guard returned at once, since prepare() had set the flag), so any default cast to the comparison type below it aborted the BE with Check failed: _open_finished in Cast; it now opens them, as EqualityDeletePredicate does. On reachability: the history needs a v3 table, a nested field added with an initial default through the Iceberg Java API (Spark and Doris cannot set defaults), and an equality delete keyed on that field. IcebergMissingNestedPlaceholderKeyParsesTextDefaultInTombstoneType covers a TIME and a timestamptz_ns default under NULL and non-NULL parents, against Parquet TIME, ORC TIME and Parquet timestamptz_ns tombstones.
TPC-H: Total hot run time: 28361 ms |
TPC-DS: Total hot run time: 151199 ms |
ClickBench: Total hot run time: 24.66 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…n their physical types ### What problem does this PR solve? Issue Number: None Related PR: apache#69711 Problem Summary: The FE sends an Iceberg primitive Doris cannot represent (TIME, timestamptz_ns) as a STRING placeholder, with Iceberg's text form as its initial default. Comparing equality-delete keys in the wider of the delete file's and the data file's types rejected two valid histories with NotSupported: 1. A TIME key across formats. ORC stores an Iceberg TIME as a LONG annotated iceberg.long-type=TIME, read as BIGINT, while Parquet's TIME(MICROS) reads as TIMEV2(6); neither widens to the other, so every scan of such a data file failed (the column cannot be projected, so the key is always hidden). Before the per-file comparison types both sides were compared as STRING ("3600000001" vs "01:00:00.000001"), so only NULL tombstones matched. 2. A missing key whose non-NULL default is the placeholder text, e.g. a nested TIME payload.k added with the default 00:00, against TIMEV2 or TIMESTAMPTZ tombstones: STRING vs the tombstone type failed before the NULL-parent masking was built, for every older data file. Under a STRING table type, a BIGINT/TIMEV2 pair now compares as TIMEV2(6), the BIGINT side converted exactly as microseconds from midnight by a new IcebergTimeMicrosExpr (CAST(BIGINT AS TIME) reads HHMMSS digits), failing outside 00:00:00..23:59:59.999999. Such a non-NULL default is parsed in the tombstones' type (TIMEV2, TIMESTAMPTZ; an ORC TIME tombstone uses TIMEV2(6)) by the strict comparison cast. Every other incomparable pair still fails. AncestorNullDefaultExpr::open now opens its children: the RETURN_IF_ERROR_OR_PREPARED guard returned at once after prepare(), so a default cast to the comparison type under a NULL-masking parent was never opened and aborted the BE with "Check failed: _open_finished" in Cast. ### Release note None ### Check List (For Author) - Test: Unit Test - IcebergEqualityDeleteComparesTimeKeysAcrossOrcAndParquet (all four ORC/Parquet data/delete pairs, NULL and non-NULL tombstones) and IcebergMissingNestedPlaceholderKeyParsesTextDefaultInTombstoneType (Parquet TIME, ORC TIME and Parquet timestamptz_ns tombstones against a nested text default under NULL and non-NULL parents); 278 tests in IcebergV2ReaderTest and the equality-delete, column mapper and evolution-cast suites pass - Behavior changed: Yes. Such Iceberg equality deletes are applied instead of failing the scan. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the third review round in
BE unit tests on macOS arm64 (ASAN), |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed the complete aggregate PR at 3c2a8e9. One new P2 correctness finding: the missing timestamptz_ns default parser can collapse distinct instants and delete rows that should survive. The targeted widening repairs otherwise follow the intended per-file comparison design.
Critical checkpoint conclusions:
- Goal and evidence: Per-data/delete-file domains address stale table types and lossy narrowing; the 14 added BE tests cover the reported histories. Exact equality remains incomplete for fractional zoned-nanosecond defaults, as detailed inline.
- Scope and abstraction: Five focused files; table semantics consume format-neutral schema/block contracts. The new EvolutionCast option defaults to existing behavior for other callers.
- Concurrency and locks: The operator-owned scan cache is shared by scanners; per-entry locks serialize immutable filter creation. No new threads, nested cache acquisition, or lock-order regression found.
- Lifecycle and initialization: Predicates retain column/hash ownership; converted expressions close through VExprContext destruction. Split state resets, const materialization, ancestor NULL propagation, and child opening were traced. No new global initialization dependency or ownership cycle.
- Configuration: No new configuration. Comparison casts deliberately override session strictness; unrelated casts retain their defaults.
- Compatibility: No storage/wire layout changes. Physical Parquet/ORC carriers, timestamp zones, integer/decimal/byte domains, defaults, and legacy metadata paths were checked; the new nanos-default precision regression is inline.
- Thrift/protobuf: No changes; cross-branch field-ID audit is not applicable.
- Parallel paths: Both widening directions, present/missing/nested keys, hidden/projected keys, split reuse, mixed-format TIME, empty deletes, and shared cloud/local native reader paths were covered. V1/JNI remain independent implementations.
- Conditions and failure behavior: New branches document concrete scenarios; empty deletes skip key casts, invalid TIME units fail, and overflow/incomparable types propagate errors. The nanos-default cast still silently loses precision.
- Test coverage: Added unit tests and adjacent integration paths were inspected statically. Missing coverage includes submicrosecond zoned defaults, composite converted keys, decimal promotion cases, actual FE/external-writer histories, and INT96 with the current timestamp-semantics marker.
- Test results: No golden output changes. Expected surviving IDs and explicit error assertions were checked against the code; no build or test was executed in this review.
- Observability: Existing prepare timers and cache/hash-memory counters remain; debug output distinguishes table/delete types. No new per-row logging or actionable observability regression found.
- Persistence and failover: No EditLog, transaction, rowset-version, or persistence changes. Upstream snapshot/delete applicability is retained; native MoW bitmap invariants are not affected.
- Writes and crashes: This is a read-path change. No new write atomicity issue or independently verified crash/leak was found; incorrect read visibility is captured inline.
- FE/BE variables: No new transmitted field; existing schema, optionality, default, and semantics metadata remain the inputs.
- Performance and memory: Tombstones convert once per cached ordered type vector; unchanged columns share immutable storage. Additional converted columns/hash indexes are intentional. No separate actionable allocation, I/O, or hot-loop regression found.
- Other concerns: Field IDs, nested child ordinals, file-local block positions, NULL constants, mapper reconciliation, residual deletes, and final materialization were traced end to end.
Existing threads: INT96 carrier selection (4395446119), NULL struct-default leaf (4395446127), empty delete files (4395446137), mixed ORC/Parquet TIME (4398396743), and placeholder-default type rejection/child opening (4398396750) have corresponding repairs and focused tests in this head; they are not reposted. The inline finding is a distinct precision defect introduced by the placeholder parsing repair. No existing P0/P1 remains to carry in existing_blocking_comment_ids.
User focus: No additional focus was supplied; the whole PR was reviewed.
Coverage limits: Static source review only, including FE metadata producers and primary Iceberg 1.12 schema/delete-planning contracts. No builds, unit/regression tests, benchmarks, or external-writer execution were performed. Test gaps above remain validation limits, with no unresolved concrete candidate. Review converged after two rounds.
Review converged after 2 round(s).
| switch (remove_nullable(delete_type)->get_primitive_type()) { | ||
| case TYPE_TIMEV2: | ||
| case TYPE_TIMESTAMPTZ: | ||
| return remove_nullable(delete_type); |
There was a problem hiding this comment.
[P2] Preserve nanoseconds in missing timestamptz_ns defaults
In an Iceberg v3 table, write id-only Parquet data, then add optional k timestamptz_ns with initial default 1970-01-01 00:00:00.000000100+00:00, and commit a newer equality delete on k = epoch. The old file has no k statistics, so the delete reaches BE. FE preserves the default as STRING, but this branch selects the tombstone's TIMESTAMPTZ(6). parse_fractional_seconds rounds 100 ns to 0 us even in strict mode, so the default now equals the epoch tombstone and SELECT id silently loses surviving rows; previously their STRING values differed. A 500 ns default against a matching 500 ns tombstone also diverges: the default rounds to 1 us while Parquet decoding floors the tombstone to 0 us. Preserve exact nanosecond identity or explicitly reject an inexact comparison domain, and extend the placeholder-default test with submicrosecond values. This is value loss introduced by the default-parsing repair, distinct from the earlier STRING/physical-type rejection.
There was a problem hiding this comment.
Confirmed, and fixed in 0d70d8c3718, by rejecting this comparison rather than parsing the default more precisely, because the precision is already gone on the tombstone side. The V2 readers carry every timestamptz_ns value as TIMESTAMPTZ(6): Parquet floors the nanoseconds (parquet_timestamp_micros) and ORC rounds them half-up (round_orc_timestamp_to_microseconds). The 500 ns example is that loss, and it also hits a microsecond-exact default: with the default 1970-01-01 00:00:00+00:00, a tombstone 100 ns after the epoch reads as the epoch and deletes every row of the files written before k was added. So no parsing of the default can tell a tombstone equal to it from one in the same microsecond.
On the previous behaviour: the STRING comparison kept these rows only because it almost never matched a non-NULL tombstone. It compared Iceberg's text of the default with the tombstone cast to text, which has six fractional digits and the session's offset, so a tombstone equal to the default was silently not applied either. The previous head of this PR failed such a split with NotSupported, and the parsing added in the third round turned that error into these silent results.
A non-NULL initial default of a timestamptz_ns key (a TIMESTAMPTZ tombstone behind the STRING placeholder) now fails the split with NotSupported again, with a message naming the field and the data file. This narrows the change made for 4398396750: TIME defaults are still parsed, which is exact, since an Iceberg TIME is microseconds both in its text (at most six fractional digits) and in the files. NULL defaults are unaffected. A key the data file has still compares as TIMESTAMPTZ(6), as on master, which compares the same microsecond values through the STRING placeholder; making that exact needs a nanosecond carrier in both readers and is outside this PR. IcebergMissingPlaceholderTimestampTzNsDefaultFailsTheScan covers a nested timestamptz_ns key with the defaults ...0000001, the epoch and ...0000005 against an epoch, a 100 ns and a 500 ns tombstone, and a key without a default (a NULL tombstone still deletes, an epoch one does not).
TPC-H: Total hot run time: 28552 ms |
TPC-DS: Total hot run time: 151100 ms |
ClickBench: Total hot run time: 24.38 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…-delete keys ### What problem does this PR solve? Issue Number: None Related PR: apache#69711 Problem Summary: A data file written before an Iceberg timestamptz_ns column k was added does not store k, so an equality delete keyed on k compares the column's initial default. The FE sends timestamptz_ns as a STRING placeholder with the default as Iceberg's nanosecond text, and the previous commit parsed that text in the tombstones' type, TIMESTAMPTZ(6). That cannot compare exactly: V2 reads the tombstones as TIMESTAMPTZ(6), Parquet flooring and ORC rounding the nanoseconds, and the strict cast rounds the 7th fractional digit of the default half up. With Parquet tombstones, the default 1970-01-01 00:00:00.0000001+00:00 matched an epoch tombstone and deleted every row of such a file, the default epoch matched a tombstone 100 ns later, and the default ...0000005 missed its own tombstone. Before that commit this history failed the scan with NotSupported. A non-NULL timestamptz_ns default behind the STRING placeholder fails the scan with a dedicated NotSupported again. TIME defaults (at most six fractional digits, exact in TIMEV2(6)) are still parsed; NULL defaults and keys the data file stores are unchanged. ### Release note None ### Check List (For Author) - Test: Unit Test - BE UT IcebergV2ReaderTest (116 tests) pass. New IcebergMissingPlaceholderTimestampTzNsDefaultFailsTheScan: the three defaults above fail with NotSupported, and a key without a default still matches NULL tombstones only; it fails when the defaults are parsed again. The timestamptz_ns case of IcebergMissingNestedPlaceholderKeyParsesTextDefaultInTombstoneType is removed. - Behavior changed: Yes. Such scans fail instead of deleting rows by an inexact comparison. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the fourth review round in 0d70d8c.
The description is updated to match. |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of PR #69711 at 0d70d8c3718eb82ab5cf884c1e715e877333ad17: no new actionable finding. All five changed files and the affected schema-to-delete-predicate path were reviewed. Both complete-review agents and the dedicated risk reviewer returned NO_NEW_VALUABLE_FINDINGS; the main reviewer independently checked their evidence. Review converged in one round.
Critical checkpoint conclusions:
- Goal and correctness: per-data-file comparison domains avoid narrowing delete keys into the scan-wide table type. Numeric, decimal, date/nanosecond, binary, TIME, timestamp, missing/default and NULL paths were traced. Promotion/default checks used the Iceberg specification.
- Scope and reuse: changes stay within the Iceberg V2 reader, its tests, and an opt-in EvolutionCast strict flag; existing cast and expression interfaces are reused.
- Concurrency and locks: split progression is sequential per scanner. FileScanOperatorX shares the cache across scanner instances; per-entry initialization is synchronized, shard-map locking is short, and raw/converted cache lookups do not nest locks. Published blocks and indexes remain immutable.
- Lifecycle and memory: predicates retain owning column/index references; temporary conversion contexts close through RAII; split teardown releases request contexts. Clone preserves strict options, and default expressions now open their children. No new global initialization dependency was found.
- Configuration: none added. Existing timezone/type-mapping settings remain shared scan inputs; strict comparison casts do not change other callers' defaults.
- Compatibility: no on-disk, serialized block or wire-format change. Incomparable domains fail explicitly. Non-NULL missing timestamptz_ns defaults deliberately return NotSupported because their tombstones have already lost nanosecond precision.
- Thrift/Protobuf IDs: no gensrc changes; cross-branch field-ID comparison is not applicable.
- Parallel paths: Parquet and ORC V2 carriers, hidden/projected keys, missing roots/children, and cached/raw filters were checked. V1 is outside this reader-local fix; Paimon's existing EvolutionCast truncation behavior is preserved.
- Special conditions: new branches explain concrete failures, including empty deletes, TIME microsecond carriers, default precision, strict overflow and child-expression opening. Errors propagate as non-OK Status rather than creating NULL matches.
- Test coverage: inspected all 14 added tests and nearby existing mapping, nullable-parent, UUID/binary, timestamp and cache tests. They cover the stated histories, mixed widths/splits, empty deletes, missing defaults, TIME format pairs, overflow and rejected domains. No builds or tests were executed under this static-only contract.
- Expected results: inspected survivor IDs and error assertions agree with the traced domains. No generated result files were changed.
- Observability: debug output includes delete-file key types; conversion misses update existing index-memory accounting; new errors identify the field/file or expression. No additional logging issue was found.
- Persistence and failover: no EditLog, metadata persistence or transaction-state changes.
- Writes and atomicity: this is read-side delete application; commit/write atomicity is unchanged. Snapshot/delete eligibility remains upstream.
- FE-BE propagation: no new transmitted variable; existing field IDs, schema/default metadata and mapping options are consumed.
- Performance: equal types avoid conversion; widened tombstones and their index are cached per original filter and ordered comparison-type vector. Extra retained variants are a deliberate memory cost. No benchmark or runtime memory measurement was performed.
- Additional risks: checked nullable constants, COW ownership, multi-key ordering, failed conversion admission, cache identity and teardown. No distinct introduced defect was substantiated.
- User focus: review_focus.txt contains no additional focus points.
Existing review context: all six original inline findings are P2, with no existing P0/P1 to carry as a blocker. The INT96 type-pair rule, NULL-leaf default walk, empty-delete skip, mixed-format TIME conversion, and TIME default/ancestor/open fixes are present. The latest nanosecond-default finding is addressed by explicit failure; this intentionally narrows the earlier placeholder-default fix, including files whose parents happen to be NULL. These concerns were not reposted.
Validation limits: the added INT96 test covers legacy plans without the current FE timestamp-semantics marker. The current-plan session-timezone mismatch also exists in the merge-base implementation, so it is a pre-existing compatibility limitation rather than a new finding. Dedicated mixed-key conversion, concurrent construction, conversion-failure retry and late-refresh tests are absent; those paths were traced statically. External-engine interoperability, sanitizer/runtime behavior and performance were not exercised. No unresolved introduced candidate or unreviewed changed-file scope remains.
Review converged after 1 round(s).
TPC-H: Total hot run time: 28735 ms |
TPC-DS: Total hot run time: 151496 ms |
ClickBench: Total hot run time: 24.36 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
…Iceberg table type (#69845) ### What problem does this PR solve? Issue Number: None Related PR: #69711 Problem Summary: **In short.** An Iceberg `timestamptz` column stored as Parquet INT96, the encoding Spark and Hive use for timestamps, reads shifted by the session time zone's offset in any non-UTC session: in Asia/Shanghai, 2024-01-01T00:00:00Z reads as `2024-01-01 00:00:00+08:00`, eight hours early, while Iceberg itself reads `2024-01-01 08:00:00+08:00`. Equality deletes on such a key miss their rows for the same reason. This PR has the FE mark each `timestamptz` field of the scan's schema dictionary, and FileScannerV2 then decodes the matching Parquet columns of data files and equality-delete files as instants, which is how Iceberg reads them. It came up in the review of #69711. **Background.** - Parquet INT96 has no logical annotation, so a reader cannot tell from the file whether an INT96 column holds an instant or a wall clock. Spark writes its `timestamp` type (an instant) as INT96 by default, and `add_files`, `migrate` and `snapshot` attach such files to Iceberg tables unchanged, as `timestamptz` columns. - Iceberg does not look at the file annotation at all: its Parquet readers choose the timestamp reader from the table type, so a `timestamptz` field reads INT96, and INT64 with or without the UTC annotation, as an instant. - The FE sends `parquet_timestamp_semantics_version = 1` for every file scan. Parquet scans therefore always use FileScannerV2, and INT96 decodes as a UTC wall clock (`DATETIMEV2`) unless the table format supplies the semantics. Paimon has the same ambiguity (TIMESTAMP and TIMESTAMP_LTZ share INT96) and solves it with `timestamp_is_adjusted_to_utc` on each field of its schema dictionary, which `PaimonReader` applies to the file schema. The Iceberg FE never set that field. **The problem, and what it cost.** For a `timestamptz` column, the projection casts the INT96 wall clock to `TIMESTAMPTZ` in the session time zone, so the instant moves by the session offset. Reproduced on master with an INT96 file attached like `add_files` and an Iceberg-written INT64 file holding the same instants, both in one table: | Session | Rows | Iceberg Java | Before | After | |---|---|---|---|---| | UTC | INT96 and INT64 | 2024-01-01 00:00:00 UTC | correct | correct | | Asia/Shanghai | INT64 (written by Iceberg) | `2024-01-01 08:00:00+08:00` | correct | correct | | Asia/Shanghai | INT96 (written by Spark) | `2024-01-01 08:00:00+08:00` | `2024-01-01 00:00:00+08:00`, 8 hours early | `2024-01-01 08:00:00+08:00` | The result is the same with field ids in the file, with only the table's name mapping, whatever the catalog's `enable.mapping.timestamp_tz` says (master always maps `timestamptz` to TIMESTAMPTZ), and with `enable_file_scanner_v2` on or off. The same shift breaks equality deletes, which compare the projected values: | Equality delete on a `timestamptz` key, non-UTC session | Before | After | |---|---|---| | INT96 data key, Iceberg-written tombstone of the same instant | the tombstone misses the row | the row is deleted | Wrong values and missed or extra deletes come back without any error. **How this PR fixes it.** - FE: `IcebergSchemaUtils.buildField` sets `timestamp_is_adjusted_to_utc = true` on every `timestamptz` field, at every nesting level; the dictionary also carries the hidden equality-delete keys. `timestamp` fields stay unmarked: marking them `false` would make BE read the Parquet statistics of an adjusted-UTC INT64 file as UTC wall clocks while its rows still decode as session wall clocks, so row-group pruning could drop matching rows. - BE, data files: `IcebergTableReader::annotate_file_schema` copies the marker onto the Parquet file column that `TableColumnMapper` will select for each field, using the mapper's own matchers (`find_column_by_field_id` / `find_column_by_name`) after the positional fallback ids are stamped, so field ids, the table's name mapping and positional ids all work. Like the mapper, it accepts an ID-less wrapper at every depth when `allow_idless_complex_wrapper_projection` is on. It then rewrites the column type with the helper `PaimonReader` already used, moved to `schema_history_util` as `apply_file_timestamp_semantics`. The mapping, the scan request and `ParquetReader` all see `TIMESTAMPTZ`, and INT96 decodes as an instant. Only a `DATETIMEV2` or `TIMESTAMPTZ` leaf takes the marker; a `TIMESTAMP_NS` leaf keeps its nanoseconds. Columns without a marked leaf keep the types the file reader reported, and a plan from an older FE carries no marker and reads as before. - BE, equality deletes: three kinds of column are read without a `ColumnMapping`: a hidden key column, the parent struct that is read for the NULLs of a nested key the data file lacks, and the key columns of an equality-delete file. Their projections now carry the markers of the annotated file column (`equality_delete_root_projection`, built on `attach_timestamp_semantics`, which the mapper now exports). Without them Parquet would decode an INT96 leaf as `DATETIMEV2` under a `TIMESTAMPTZ` file schema, and the read would fail its type check. That holds even when the query projects the column, because reading it as a predicate column replaces the projection its mapping marked. Equality-delete files get the same annotation, so tombstones and data keys of a `timestamptz` field are both instants. The `DATETIMEV2` / `TIMESTAMPTZ` comparison rule from #69711 stays for plans from older FEs. - Refactor: `find_current_schema` and `build_schema_identity_from_external_field` (now also carrying the marker) move to `schema_history_util`; `TableReader`'s four copies of the current-schema lookup use the helper. **Results.** - FE unit test `IcebergSchemaUtilsTest#timestamptzFieldsCarryInstantMarkerAtEveryLevel`: top-level, struct, list and map `timestamptz` fields are marked; `timestamp` and `timestamp_ns` fields are not, under both values of the mapping flag. - 5 new BE unit tests in `iceberg_reader_test.cpp`, all failing without the BE change: the file-schema annotation (field ids at every level, `TIMESTAMP_NS` untouched, ORC untouched); INT96 `timestamptz` in Asia/Shanghai with field ids, name mapping and positional ids, with and without the marker; INT96 inside a struct and a list; an INT96 equality key against an Iceberg tombstone, hidden or projected; and the DST fall-back above. Leaving the delete files unannotated fails the DST case (the second tombstone deletes the first row); projecting a hidden key or a tombstone without its marker fails both equality-delete cases with a read error; marking a `TIMESTAMP_NS` leaf fails the annotation case. With neither data nor delete files annotated (the reader before this PR), the INT96 key deletes nothing and the DST case deletes both rows. The existing case for an older FE's plan (no marker) still passes. - 2 more BE unit tests from review: - INT96 under a nested ID-less wrapper struct, read and used as an equality key. It fails when the walk accepts the wrapper only at the root. - A nested equality key missing from an older file whose parent struct holds an INT96 `timestamptz`. It checks the survivors with the parent hidden and projected, that the projected timestamp reads as an instant in Asia/Shanghai, and a plan without the marker. It fails with the reader's type check when the parent is read without the marker. - End to end on a local cluster: an Iceberg table holding an INT96 Parquet file attached like `add_files` (one table with field ids in the file, one relying on `schema.name-mapping.default`) and an Iceberg-written INT64 file with the same instants, read through three catalogs (`enable.mapping.timestamp_tz` unset, `true`, `false`), with `enable_file_scanner_v2` on and off, in UTC and Asia/Shanghai: 24 combinations. Before this PR the INT96 rows were 8 hours early in every Asia/Shanghai combination (8 of the 16 measured); with it the INT96 rows equal the INT64 rows in all 24, and the INT64 rows and the UTC results are unchanged. branch-4.1 and branch-4.2 have the same code. **Classes, and how they call each other.** - `IcebergSchemaUtils` (FE): builds the schema dictionary; now marks `timestamptz` fields. - `schema_history_util`: `find_current_schema`, `build_schema_identity_from_external_field`, and `apply_file_timestamp_semantics` (moved from `PaimonReader`). - `IcebergTableReader`: `annotate_file_schema` → `_apply_positional_id_fallback` (the existing logic) and `_apply_table_timestamp_semantics`; `_append_equality_delete_predicates` (hidden keys), `_build_missing_equality_delete_key_expr` (the parent of a missing nested key) and `_load_equality_delete_file` (tombstones) project their columns with the markers through `equality_delete_root_projection`. - `column_mapper`: `attach_timestamp_semantics` is exported. ``` FE IcebergSchemaUtils.buildField --timestamptz--> TField.timestamp_is_adjusted_to_utc = true (history_schema_info, current_schema_id = -1) BE TableReader::open_reader +-- IcebergTableReader::annotate_file_schema (Parquet only) | +-- _apply_positional_id_fallback | +-- _apply_table_timestamp_semantics | +-- find_current_schema, build_schema_identity_from_external_field | +-- find_column_by_field_id | find_column_by_name (the mapper's matchers) | +-- apply_file_timestamp_semantics (shared with PaimonReader) +-- TableColumnMapper: mapping/projection take the marker from the file column +-- ParquetReader::open: apply_projection_timestamp_semantics -> INT96 decodes as TIMESTAMPTZ IcebergTableReader::_append_equality_delete_predicates +-- equality_delete_root_projection -> attach_timestamp_semantics (hidden key) +-- _build_missing_equality_delete_key_expr +-- equality_delete_root_projection (parent of a missing key) IcebergTableReader::_load_equality_delete_file -> _apply_table_timestamp_semantics + equality_delete_root_projection (tombstones) ``` ORC files are not changed: ORC's file type already separates `TIMESTAMP` from `TIMESTAMP_INSTANT`. ### Release note Fix Iceberg `timestamptz` columns stored as Parquet INT96 (written by Spark or Hive and attached with `add_files`, `migrate` or `snapshot`) reading shifted by the session time zone's offset in non-UTC sessions, and equality deletes on such keys missing their rows. ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [x] Unit Test - BE: `run-be-ut.sh --run --filter='IcebergV2ReaderTest.*:Paimon*ReaderTest.*:Hudi*Test.*:TableReaderTest.*:ColumnMapper*:FlussUnionLakeReaderTest.*:EqualityDeletePredicateTest.*:*PositionDelete*'`: 532 of 532 pass on macOS arm64 (ASAN), including the 5 new cases. After the first review round, `IcebergV2ReaderTest` passes 122 of 122, including the new nested ID-less wrapper case. It fails when the walk accepts the wrapper only at the root. After the second review round, the whole filter passes 534 of 534 and `IcebergV2ReaderTest` 123 of 123, including the new missing nested key case. It fails when the missing key's parent is read without the marker. They ran on a local base; the commit was then applied to master `2a4191c1dc9` without conflicts, and the changed sources and their test files were syntax-checked against that tree with the UT build flags. - FE: `mvn -pl :fe-connector-iceberg -am package -Dtest=IcebergSchemaUtilsTest`: 35 of 35 pass, checkstyle passes. - [x] Manual test (add detailed scripts or steps below) - The local end-to-end check under Results, with an ASAN BE and the Iceberg connector built from this change. - [ ] No need to test or manual test. Explain why: - Behavior changed: - [ ] No. - [x] Yes. Parquet columns of Iceberg `timestamptz` fields decode as instants by the table type, as Iceberg reads them: INT96 no longer shifts by the session offset, an INT64 timestamp without the UTC annotation under a `timestamptz` field reads its value as UTC, and equality deletes on `timestamptz` keys compare instants. Plans from an older FE are unchanged. - Does this need documentation? - [x] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
### What problem does this PR solve? Issue Number: #69782 Related PR: #69711 Problem Summary: `NestedStructFieldExpr::open()` reuses the prepare guard and skips opening its child. Use the normal open lifecycle, completing the nested traversal fix after #69711. ### Release note Fix initialization of nested struct expressions in FileScannerV2 Iceberg equality deletes. ### Check List (For Author) - Test - [x] Unit Test: added coverage for cast defaults, nullable ancestors, and cloned contexts. - Behavior changed: - [x] Yes. Open nested expression children before execution. - Does this need documentation? - [x] No. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
### What problem does this PR solve? Issue Number: #69782 Related PR: #69711 Problem Summary: `NestedStructFieldExpr::open()` reuses the prepare guard and skips opening its child. Use the normal open lifecycle, completing the nested traversal fix after #69711. ### Release note Fix initialization of nested struct expressions in FileScannerV2 Iceberg equality deletes. ### Check List (For Author) - Test - [x] Unit Test: added coverage for cast defaults, nullable ancestors, and cloned contexts. - Behavior changed: - [x] Yes. Open nested expression children before execution. - Does this need documentation? - [x] No. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
What problem does this PR solve?
Issue Number: None
Related PR: #69536, #68267
Problem Summary:
In short. FileScannerV2 now compares an Iceberg equality-delete key in the wider of the delete file's and the data file's own key types, instead of in the one table type the FE sends for the whole scan. For some table histories that single type is wrong: the reader then deletes rows it must keep, keeps rows it must delete, or fails the scan. This follows the review of #69536, where these cases came up.
Background. An equality delete removes a data row whose key values equal a tombstone's. After a type promotion the two sides can be stored in different types: tombstones written before an INT -> LONG promotion hold INT values that must still match LONG data. #68267 made FileScannerV2 compare in the current schema's type, and for a key that the current schema no longer has, the FE sends a type recovered from the retained schemas (#69536). The reader casts both the tombstones and the data keys to that one type.
The problem, and what it cost. One type per scan cannot describe every delete file:
expireSnapshots().cleanExpiredMetadata(true)removed the schema of a schema-only INT -> LONG promotion of a dropped keyTIMESTAMP_NS overflow when casting row ...unknownas STRING2024-01-02!=2024-01-02 00:00:00.000000000, the row stays visibleThree of the four return wrong rows without any error. The V1 reader is not changed by this PR.
How this PR fixes it.
equality_delete_comparison_type). The accepted widenings are the Iceberg promotions (INT -> LONG, FLOAT -> DOUBLE, decimal precision, DATE -> TIMESTAMP and DATE -> TIMESTAMP_NS), a wider fractional scale of the same date-time kind, wider integers, STRING -> VARBINARY (an Iceberg BINARY key decodes as either one, depending onenable_mapping_varbinary), and TIMESTAMP -> TIMESTAMP_NS. The last one is not an Iceberg promotion; it is accepted for the abandoned-schema case above, because scaling microseconds to nanoseconds is exact. When the table field istimestamptz, aDATETIMEV2key (Parquet INT96, which carries no instant annotation, as Spark and Hive write it) and aTIMESTAMPTZkey compare asTIMESTAMPTZat the larger scale, theDATETIMEV2side converted with the session time zone as the projection converts that column. When the table field is the STRING placeholder the FE sends for an Iceberg TIME, which Doris cannot represent, an ORC TIME key (a LONG annotatediceberg.long-type=TIME, read asBIGINT) and a Parquet one (TIME(MICROS), read asTIMEV2(6)) compare asTIMEV2(6), theBIGINTside converted exactly as microseconds from midnight by a dedicated expression (CAST(BIGINT AS TIME)reads the integer's digits as HHMMSS), and a value outside 00:00:00..23:59:59.999999 fails the scan. Two types that convert in neither direction fail the scan instead of being compared in a third type._convert_equality_delete_filter). A table without type changes is compared as before, with no cast and no copy.{}whose leaf has none) the key is NULL in that file whatever its table type, so it is compared in the delete file's type (anunknownkey arrives as STRING). A non-NULL default behind the STRING placeholder of an Iceberg TIME is Iceberg's text of the value (00:00), with at most six fractional digits, so it is parsed exactly in the tombstones' type,TIMEV2(TIMEV2(6)against an ORC TIME tombstone), by the strict comparison cast. A non-NULL default of atimestamptz_nskey fails the scan withNotSupported: V2 reads its tombstones asTIMESTAMPTZ(6)(Parquet floors the nanoseconds, ORC rounds them), so no parse of the nanosecond default compares exactly; the default epoch would match a tombstone 100 ns later and delete every row of the file.AncestorNullDefaultExpr, which gives a missing child the NULL of its NULL parent and its default otherwise, now opens its children. Itsopen()returned before opening them, becauseprepare()had already set the flag its guard checks, so a default cast to the comparison type below it was never opened and aborted the BE withCheck failed: _open_finishedinCast.strictflag onEvolutionCast: a non-strict overflow would turn a key into NULL and let a NULL tombstone delete it. DATE -> TIMESTAMP_NS was already strict; otherEvolutionCastusers are unchanged.Results.
iceberg_reader_test.cpp: the four histories above; INT tombstones against LONG, INT and LONG data files of one scan (the third split reuses the cached widened copy); a missing key's INT default against a LONG tombstone; a missing key without a default under anunknowntable type; a microsecond key outside the nanosecond range, which must fail; and incomparable key types. With the reader before this PR, 6 of them fail; the other three pin behavior the old code shares. Removing the TIMESTAMP -> TIMESTAMP_NS widening fails 2, and making the casts non-strict fails 1. Three more came from review: an INT96 data key against an adjusted-UTC tombstone in Asia/Shanghai, a NULL leaf under a struct default{}, and an empty TIMESTAMP_NS delete file against a DATE key; removing the INT96 rule, the NULL-leaf walk or the empty-file skip fails its test. Two more came from the third round: Iceberg TIME keys across all four ORC / Parquet data and delete file pairs, with NULL and non-NULL tombstones, and a nested Iceberg TIME key added with a text default, against Parquet TIME and ORC TIME tombstones, under NULL and non-NULL parents. Removing the TIME pair rule or the default parsing, or converting the ORC TIME with a SQL cast, fails them; leavingAncestorNullDefaultExpr's children unopened aborts. One more came from the fourth round: a nestedtimestamptz_nskey with the defaults...0000001, the epoch and...0000005fails against an epoch, a 100 ns and a 500 ns tombstone, while a key without a default still matches NULL tombstones only; parsing those defaults again fails it.file://Hadoop catalog, built with the Iceberg 1.12.0 Java API:k INTwith rows (1, NULL), (2, 0), (3, 1);kpromoted to LONG without a snapshot; a delete ofk = 4294967296written under that schema;kdropped; the delete committed; thenexpireSnapshots().retainLast(2).cleanExpiredMetadata(true), which leaves schema 0 (k INT) and schema 2 (nok).SELECT idthrough FileScannerV2 returned 2, 3 before this PR and returns 1, 2, 3 with it.branch-4.1 and branch-4.2 have the same FileScannerV2 code.
Release note
Fix FileScannerV2 comparing Iceberg equality-delete keys in a single scan-wide type, which could delete or keep the wrong rows, or fail the scan, after a key's type changed (for example a schema-only promotion removed by
cleanExpiredMetadata, or a key whose retained schemas only have it asunknown).Check List (For Author)
run-be-ut.sh --run --filter='IcebergV2ReaderTest.*:*EqualityDelete*:*ColumnMapper*:*EvolutionCast*:*TableReader*': 526 of 526 pass on macOS arm64 (ASAN), including the 14 new cases.43f9ceb48d7: [refine](expr) Bind CAST executors for direct column execution #69639 moved the strict-mode override ofEvolutionCastfromprepare()intoconfigure_cast_options(), so the newstrictflag now setsCastOptions::strict_modethere, next to the existing DATE -> TIMESTAMP_NS rule. The run above is on the content before the rebase (the third-round commit applies to both unchanged); the 67 TUs that this PR changes or that master changed since5caaeb3e3d4were syntax-checked against the rebased tree with the UT build flags, and CI runs the suites again.Check List (For Reviewer who merge this PR)