Repository navigation
[fix](be) Parse complex Iceberg initial defaults at full precision - #69369
Merged
morningman merged 1 commit intoOct 23, 2026
Merged
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: Iceberg stores the initial default of a struct, list or map field as single-value JSON. Both BE readers, FileScanner V1 (iceberg_default_value.h) and V2 (format_v2/table/iceberg_reader.cpp), parse that JSON with rapidjson's default Parse. Its fast path rounds a long decimal significand to a double and then scales it, so a double with 17 or more significant digits can be rounded twice and come back one ULP off: [0.18172760479972437302] decodes to 0x3fc742d9a3b296db instead of the correctly rounded 0x3fc742d9a3b296dc that Spark returns. Rows from data files written before the column existed then read a different value in Doris than in Spark. branch-4.1 apache#66538 parses this JSON with kParseFullPrecisionFlag in both readers. This change does the same and ports its two unit tests. ### Release note Fix: a DOUBLE inside the initial default of an Iceberg struct, list or map column could be read one ULP off; it now matches Spark exactly. ### Check List (For Author) - Test: Unit Test - IcebergReaderTest.materializes_complex_double_initial_default_at_full_precision and IcebergV2ReaderTest.AnnotateBuildsComplexDoubleDefaultAtFullPrecision fail without the fix (0x3fc742d9a3b296db) and pass with it; all 144 tests of IcebergReaderTest and IcebergV2ReaderTest pass - Behavior changed: Yes. Doubles inside complex initial defaults read back bit-identical to Spark instead of possibly one ULP off - Does this need documentation: No Co-authored-by: daidai <[email protected]> Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No actionable defect found in PR #69369 at 9af6e56. All four changed paths were reviewed, including both Iceberg reader implementations, their default-materialization callers, and both new tests. No existing inline findings or additional user focus were supplied; a fresh PR comment readback also found none.
The changed parser mode agrees with RapidJSON's full-precision contract and the affected recursive representations in Iceberg's JSON single-value serialization. Independent review traced FE serialization through recursive JSON conversion, numeric SerDe, owned columns/literals, and mapper/default consumers.
Required checkpoint conclusions:
- Goal and evidence: both complex-default JSON entry points now request full precision. Each added test reaches its production entry point and checks stored DOUBLE bits. Exact rational arithmetic independently confirms the expected 0x3fc742d9a3b296dc value.
- Scope: two parser-flag changes and two focused tests with their includes; no unrelated changes.
- Concurrency and locks: parsing remains synchronous with stack-local DOM/scratch; no new shared state, threads, locks, or lock-order changes.
- Lifecycle and static initialization: values are inserted into owned columns before temporary storage expires. The V2 test materializes its const wrapper before inspection. No new static initialization or ownership cycle.
- Configuration: no configuration changes.
- Compatibility: existing schema transport, format representation, version gates and function signatures remain intact. The intended observable change is corrected numeric rounding.
- Thrift/protobuf: no definitions changed; field-ID compatibility audit is not applicable.
- Parallel paths: both V1 and V2 are updated. STRUCT, ARRAY and MAP recursion, including nested field-level defaults, reaches the corrected parser. Top-level scalar defaults retain their separate SerDe path.
- Conditional checks and errors: no new branches; existing parse, schema, nullability and conversion failures still return non-OK Status.
- Test coverage: two exact-bit list cases added. Array child nullable casts follow DataTypeArray's normalization; C++20 supports bit_cast. Neighboring complex/default/error/ownership tests were inspected. New executable cases do not independently cover every container and floating boundary.
- Expected results: the bit oracle is correct; no generated .out files changed.
- Observability: existing contextual errors remain available; this local setup correction introduces no execution stage needing new counters.
- Transactions, persistence and failover: no related logic changed; transaction visibility, versions and delete bitmaps are unaffected by this diff.
- Writes and atomicity: read-time default construction only; no write or crash-recovery protocol changes.
- FE-BE variables: no new variables; existing recursive default transport remains sufficient.
- Performance: additional precision work occurs during default preparation with existing value reuse, rather than introducing parsing per output row. No new I/O, repeated schema traversal or copy loop was found.
- Other issues: the final recheck covered numeric reserialization, default flags, signed zero, subnormals, non-finite values, integer/decimal behavior, nullable shapes and parallel consumers; no additional defect was substantiated.
Validation: the pure-text BE build-hygiene gate passed header layering/budgets/PCH, extern-template pairing and unity-skip coverage. No Doris build, unit/regression test, external-writer integration run, runtime benchmark or compiled ABI audit was performed under this review-only contract. The author's reported test execution was not independently rerun. Broader floating/container behavior was assessed through source and library contracts.
Convergence: all three assigned reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged empty inline set in round 1. Both initial risk items were dismissed with concrete evidence; no unresolved candidate or uncovered changed path remains. New inline findings: 0. Existing P0/P1 blockers confirmed: 0.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28585 ms |
Contributor
TPC-DS: Total hot run time: 151075 ms |
Contributor
ClickBench: Total hot run time: 24.27 s |
4 of 16 tasks
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
hubgeter
approved these changes
Oct 23, 2026
Gabriel39
approved these changes
Oct 23, 2026
morningman
added a commit
that referenced
this pull request
Oct 23, 2026
…rve TIcebergFileDesc field 13 (#69394) ### What problem does this PR solve? Issue Number: None Related PR: #66538 #65851 Problem Summary: **In short.** This ports the Iceberg V3 default-value tests of branch-4.1 #66538 to master: 3 BE unit tests, 37 FE unit tests and one regression check, all for behavior master already has. It also reserves field id 13 of `TIcebergFileDesc`, which branch-4.1 and branch-4.2 use for `equality_delete_schema`. No product code changes. **Background.** Iceberg V3 initial / write defaults were implemented twice: by #65851 on master and by #66538 on branch-4.1, which branch-4.2 inherits. The 4.1 tests target fe-core classes (`IcebergScanNode`, `IcebergWriteSchemaContext`, `IcebergTransaction`, …) and V1 reader internals that master no longer has. On master the Iceberg FE lives in the `fe-connector-iceberg` plugin and the V1 reader in `iceberg_reader_mixin.h`. So most of the 4.1 tests never reached master, even where master implements the same behavior. **The problem, and what it cost.** A scenario-by-scenario comparison of the two test suites found that master implements most of the 4.1-tested behavior with no test of its own. The untested behavior: | Area | Untested on master before this PR | |---|---| | Equality-delete schema carrier | a dropped key keeps its latest name, optionality and initial default; a dropped nested key comes back inside its parent; a dropped nanosecond key stays `TIMESTAMP_NS`; only referenced historical fields enter the carrier | | Rolling-upgrade fences | missing-required-field and nested-initial-default fences follow the projection; collection wrappers do not trigger them; schema-only targets and cherry-picked ancestry are covered; name-mapping alias collisions | | Write commit fences | schema skew, a recreated table and current-spec replacement are rejected before a transaction opens; a required column, a spec drift to unpartitioned or a table replacement that lands while the commit stages its snapshot is rejected at the final metadata swap | | Pinned writer metadata | INSERT commit, merge sink (including the unpartitioned delete spec id) and file conversion use the statement-pinned spec, sort order and format, not the live table | | Write defaults | typed SQL literal per primitive type; maps and NULL elements in complex defaults; `DEFAULT` resolves by field id, not by a reused name; UPDATE and matched MERGE do not inject write defaults | | Wire compatibility | `TIcebergFileDesc` ends at field 12 on master, while 4.1 / 4.2 send `13: optional ExternalTableSchema.TSchema equality_delete_schema`; nothing stops master from reusing 13 for an unrelated field | Without these tests a regression in any of these paths would go unnoticed, and a reused field 13 would misdecode 4.x plans. **How this PR fixes it.** It rewrites the 4.1 scenarios against master's APIs. Plugin tests use the existing Mockito-free fakes (`InMemoryCatalog`, `Proxy` over `TableOperations`). The commit-fence tests hook `newSnapshotId` on the staging operations, so the concurrent change lands after the commit preflight and before the metadata swap. The BE tests are copied unchanged from 4.1 (the helper signatures match on master). `PlanNodes.thrift` gets a comment reserving field 13. `test_iceberg_write_default` gains a `qt_` check that `information_schema.columns.COLUMN_DEFAULT` stays NULL for a column whose default is a connector-only write default. The behavior differences found in the same comparison are fixed in separate PRs (for example #69369). **Results.** All new tests pass on master. To show they check something, the fence and pinned-metadata tests were re-run with the guarded condition removed from the product code; each failed. | Suite | Result | |---|---| | `fe-connector-iceberg` (all tests) | 1656 run, 0 failures, 0 errors, 5 skipped; checkstyle clean | | fe-core `ConnectorWriteSchemaUtilsTest`, `ExternalRowLevelUpdatePlanBuilderTest`, `ExternalRowLevelMergePlanBuilderTest` | 13 run, 0 failures | | BE `IcebergV2ReaderTest.IcebergProjectedStructDoesNotReuseReaddedEqualityKeyByName`, `ParquetReadLinesTest.iceberg_row_id_fetch_materializes_readded_missing_column`, `VOrcTransformerTest.SkipsInvalidBinaryChildrenHiddenByNullableCollections` | pass | | Regression `external_table_p0/iceberg/test_iceberg_write_default` | pass | Co-authored-by: daidai <[email protected]>
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: #66538
Problem Summary:
In short. This ports a fix from branch-4.1 #66538 (Iceberg V3 default values): both BE Iceberg readers parse the JSON form of a complex initial default with
rapidjson::kParseFullPrecisionFlag, so a DOUBLE inside it reads back bit-identical to Spark.Background. Iceberg stores the initial default of a struct, list or map field as single-value JSON, for example
[0.18172760479972437302]for alist<double>column added after some data files were written. When Doris reads such an older file, it materializes the missing column from that JSON: FileScanner V1 increate_initial_default_column(be/src/format/table/iceberg_default_value.h) and FileScanner V2 when it builds the default expression (be/src/format_v2/table/iceberg_reader.cpp). Master added both in #65851.The problem, and what it cost. Both readers call rapidjson's default
Parse. Its fast path rounds a long decimal significand to a double and then scales it by a power of ten, so a number with a long significand can be rounded twice and land on the neighbouring double.[0.18172760479972437302]read by V1 / V20x3fc742d9a3b296db0x3fc742d9a3b296dc(correctly rounded, same as Spark and Python)How this PR fixes it. The two
Parsecalls becomeParse<rapidjson::kParseFullPrecisionFlag>, as on branch-4.1 and branch-4.2. Top-level scalar defaults do not go through this JSON parse and are unchanged.Results. The two unit tests ported from #66538,
IcebergReaderTest.materializes_complex_double_initial_default_at_full_precision(V1) andIcebergV2ReaderTest.AnnotateBuildsComplexDoubleDefaultAtFullPrecision(V2), check the exact bits of the decoded double. Without the fix both fail with0x3fc742d9a3b296db; with it both pass, and all 144 tests of the two reader suites pass.Original author: @hubgeter
Release note
Fix: a DOUBLE inside the initial default of an Iceberg struct, list or map column could be read one ULP off; it now matches Spark exactly.
Check List (For Author)
Test
Behavior changed:
Doubles inside complex Iceberg initial defaults read back bit-identical to Spark instead of possibly one ULP off.
Does this need documentation?
Check List (For Reviewer who merge this PR)