[fix](iceberg) Support nested decimal precision promotion#65122
Open
Gabriel39 wants to merge 1 commit into
Open
[fix](iceberg) Support nested decimal precision promotion#65122Gabriel39 wants to merge 1 commit into
Gabriel39 wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Gabriel39
force-pushed
the
fix-doris-26324-master
branch
3 times, most recently
from
d5a3595 to
6a18fcb
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review result: request changes.
Critical checkpoint conclusions:
- Goal/test coverage: The PR adds tests for the new strict nested decimal promotion helper and Iceberg validation path, but the internal OLAP light schema-change path is not covered for decimal primitive-family boundary changes.
- Scope/focus: The implementation is localized, but sharing the helper with internal complex schema change makes Iceberg metadata-safe decimal widening affect OLAP table schema change behavior.
- Concurrency/lifecycle/config: No new threads, lifecycle ownership, or dynamic config items are introduced.
- Compatibility/parallel paths: The Iceberg path is metadata-oriented, but the parallel internal OLAP complex-type path can apply accepted changes as light schema change without rewriting stored values.
- Tests/results: I could not run the Maven tests because thirdparty/installed and thirdparty/installed/bin/protoc are missing in this runner; per fe/AGENTS.md I stopped before build/test execution.
- User focus: No additional user-provided focus points were present.
Subagent conclusions:
- optimizer-rewrite: NO_NEW_VALUABLE_FINDINGS in the final convergence round; no subagent candidates became comments.
- tests-session-config: NO_NEW_VALUABLE_FINDINGS in the final convergence round; no subagent candidates became comments.
- Main accepted MAIN-1 as the only inline comment after duplicate checks; no existing inline threads were present.
Gabriel39
force-pushed
the
fix-doris-26324-master
branch
from
6a18fcb to
6fa8099
Compare
Doris validates Iceberg complex column modifications before committing the Iceberg schema update. The previous nested primitive promotion rules covered varchar length growth, exact matches, integer widening, and float to double, but missed Iceberg decimal precision widening. Split the validation paths so internal table complex type schema change keeps the existing nested promotion semantics, while Iceberg complex column schema evolution can additionally allow decimal precision widening with unchanged scale. Keep the shared strict nested primitive promotion utility limited to integer widening and float to double. Add an Iceberg-specific nested decimal promotion helper for decimal(P, S) -> decimal(P', S), where P' >= P and S is unchanged. Scale changes remain rejected because they would reinterpret historical decimal values. Add ColumnTest coverage to verify internal table nested decimal precision widening is still rejected and Iceberg decimal promotion rules are isolated. Add IcebergMetadataOpsValidationTest coverage for array, map value, and struct nested decimal promotion plus narrowing and scale-change rejection through the Iceberg ALTER validation path. Tests: mvn test -pl fe-core -am -Dcheckstyle.skip=true -DfailIfNoTests=false -Dmaven.build.cache.enabled=false -Dtest=ColumnTest,IcebergMetadataOpsValidationTest
Gabriel39
force-pushed
the
fix-doris-26324-master
branch
from
6fa8099 to
751470a
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Review result: request changes.
Critical checkpoint conclusions:
- Goal/test coverage: The implementation splits internal complex-type schema-change validation from the Iceberg validation path and keeps internal nested decimal precision promotion rejected, but the new Iceberg tests only cover the struct-field shape while production also supports array-element and map-value updates through separate code paths.
- Scope/focus: The code changes are localized to
ColumnType,IcebergMetadataOps, and focused FE tests. - Concurrency/lifecycle/config: No new threads, locks, lifecycle ownership, or dynamic config items are introduced.
- Compatibility/parallel paths: Internal OLAP callers still use the strict helper; Iceberg modify-column validation uses the decimal-enabled helper. Map-key type changes remain rejected by
applyMapChangebefore commit. - Tests/results: Static review only.
git diff --checkpassed for the four live PR files after fetching the base SHA. I did not run FE tests because.worktree_initializedis absent andthirdparty/installed/bin/protocis missing, which is a stop condition underfe/AGENTS.md. - User focus: No additional user-provided review focus was present.
Subagent conclusions:
- optimizer-rewrite: No candidates became comments; convergence round 1 returned
NO_NEW_VALUABLE_FINDINGS. - tests-session-config:
TSC-1was accepted asMAIN-1and submitted as the inline comment; convergence round 1 returnedNO_NEW_VALUABLE_FINDINGSafter the main ledger update. - Existing review thread 3506222061 was treated as already-known duplicate context and was not resubmitted.
| } | ||
|
|
||
| @Test | ||
| public void testValidateForModifyComplexColumnAllowsNestedDecimalPrecisionPromotion() throws Throwable { |
Contributor
There was a problem hiding this comment.
This still only exercises the new decimal promotion through a struct field. The production path this PR enables is recursive for all complex shapes, and after validation IcebergMetadataOps takes different update paths for arrays and maps: array elements go through applyListChange(... updateColumn(elementPath, ...)), while map values go through applyMapChange(... updateColumn(valuePath, ...)). A struct-only reflection test would not catch a bad element/value path or an UpdateSchema incompatibility in those two supported shapes. Please add Iceberg modify validation coverage for at least array<decimal(5,3)> -> array<decimal(10,3)> and map<int,decimal(5,3)> -> map<int,decimal(10,3)> (ideally through the full modify/apply path, not only the shared helper).
Contributor
TPC-H: Total hot run time: 29752 ms |
Contributor
TPC-DS: Total hot run time: 173954 ms |
Contributor
ClickBench: Total hot run time: 25.23 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
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
2 participants
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.
Proposed changes
Fix Iceberg complex column schema evolution validation for nested decimal precision widening.
Doris validates complex column modifications before committing Iceberg schema updates. The previous nested primitive promotion rules covered varchar length growth, exact matches, integer widening, and
FLOAT -> DOUBLE, but missed Iceberg's valid decimal promotion rule:This patch keeps the internal table complex type schema-change path on the existing strict nested promotion rules:
FLOAT -> DOUBLEIceberg complex column schema evolution uses a separate validation entry point that additionally accepts decimal precision widening when both sides are decimal, scale is unchanged, and destination precision is not smaller. This keeps internal table nested decimal schema-change behavior unchanged.
Why
The internal table schema change matrix is broader than Iceberg schema evolution semantics. It allows conversions such as decimal bucket changes, date/string conversions, and other casts that are not safe metadata-only promotions for Iceberg nested schema evolution.
Tests
mvn test -pl fe-core -am \ -Dcheckstyle.skip=true \ -DfailIfNoTests=false \ -Dmaven.build.cache.enabled=false \ -Dtest=ColumnTest,IcebergMetadataOpsValidationTestRelated issue
DORIS-26324