Repository navigation
[fix](iceberg) Honor standard row-level isolation properties - #69702
Merged
Gabriel39 merged 4 commits intoNov 4, 2026
Merged
Conversation
### What problem does this PR solve? Related PR: apache#69551 Problem Summary: Iceberg DELETE, UPDATE, and MERGE all read the non-standard delete_isolation_level property, so the standard write.delete/update/merge.isolation-level table properties were ignored. Tables configured for snapshot isolation therefore still performed serializable data-file conflict validation and rejected normal concurrent appends. The same non-standard property also incorrectly applied to every row-level operation. Select the Iceberg TableProperties key by WriteOperation and parse it with IsolationLevel.fromName. Preserve all existing row-delta validation except validateNoConflictingDataFiles, which remains gated on serializable isolation. Enable the three snapshot-isolation regressions and cover per-operation key selection, defaults, and invalid values. ### Release note Iceberg DELETE, UPDATE, and MERGE now honor their standard per-operation isolation-level table properties. ### Check List (For Author) - Test: FE unit tests - mvn -f fe/pom.xml -pl :fe-connector-iceberg -am test \ -Dmaven.build.cache.enabled=false - 1748 tests run; 4 unrelated GCS/Glue authentication tests failed with upstream AWS HTTP 503 responses. All transaction and isolation tests passed. - Behavior changed: Yes, snapshot isolation now permits concurrent data-file appends while retaining delete-file and referenced-data-file validation. - Does this need documentation: No
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.
One verified P1 requires changes: SQL UPDATE is still translated as MERGE, so the new selector uses the MERGE isolation property for UPDATE. With MERGE=snapshot and UPDATE=serializable (including its default), UPDATE can newly skip conflicting-data-file validation. The inverse settings also leave the promised UPDATE snapshot behavior broken. The inline finding describes the complete caller chain and required operation propagation fix.
Reviewed all three changed files, their SQL-to-sink callers, both RowDelta builders, transaction publication/retry/cleanup, and the pinned Iceberg 1.12.0 APIs. No existing inline findings were supplied or found in the PR-scoped readback; existing_blocking_comment_ids is empty. No additional user focus was supplied.
| Critical checkpoint | Conclusion |
|---|---|
| Goal and proof | Standard per-operation isolation selection is correct for the supplied enum, but the SQL UPDATE path violates the goal (P1). New tests inject the enum directly and miss this path. |
| Scope and clarity | Small, focused change; reuses SDK constants and parser. |
| Concurrency and locks | Independent catalog writers race with the SDK commit/replay. Wrong UPDATE identity changes validation (P1); no new threads, lock work, lock order, or deadlock path. |
| Lifecycle/static initialization | Existing begin/commit/close and table leases remain intact; no new resource ownership or static initialization. |
| Configuration/dynamic changes | Uses existing persisted Iceberg table properties, read from transaction metadata; no new FE config or restart requirement. |
| Compatibility/rolling upgrades | No SPI, wire, symbol, or storage-format change. The old nonstandard delete_isolation_level key no longer controls writes; legacy-only tables use standard defaults. |
| Thrift/protobuf | No definition or field-ID changes. |
| Parallel paths | DELETE and SQL MERGE carry the correct operation; UPDATE shares their merge sink but loses its identity. INSERT/OVERWRITE/REWRITE are unaffected. Whole-file metadata DELETE remains its existing separate path. |
| Special conditions | Only serializable requests conflicting-data-file validation; base snapshot, conflict filter, delete-file, and referenced-file validations remain enabled. Invalid values throw. |
| Test coverage | Connector helper tests cover defaults, distinct keys, invalid input, and concurrent-append acceptance; enabled tests use real SDK metadata commits with v2 delete fragments. SQL translation and v3 snapshot behavior are not newly exercised. |
| Test results | No golden results changed. Tests/builds were not run under the static-only contract; author-reported passes were not independently reproduced. |
| Observability | Existing commit exception wrapping surfaces parser/conflict failures; no new metrics/logging requirement identified. |
| Persistence/failover | Remote SDK metadata publication, retry, and cleanup remain unchanged; no new Doris EditLog state or failover path. Crash/failover behavior was inspected, not executed. |
| Writes/atomicity/crashes | Data and delete files still publish in one SDK RowDelta/transaction. Remaining guards survive retries, but UPDATE isolation is weakened in the P1 scenario. No new crash-cleanup path. |
| FE-to-BE propagation | No new transmitted variable; the faulty operation identity is in the existing FE write handle. |
| Performance | One constant-time property lookup/switch and enum parse per commit; no new IO, unbounded work, or significant memory allocation. |
| Other issues/core invariants | No further introduced issue found. Native partition versions, MoW delete bitmaps, and BE allocation tracking are unchanged. The inherited empty-read -1 ancestry fence was verified and excluded as pre-existing. |
The property names/defaults and strict case-insensitive parsing match the pinned TableProperties and IsolationLevel sources. The retained checks are independent in BaseRowDelta.
Converged after 2 review rounds: all three reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged final set, required static coverage is complete, and every candidate/risk has an evidence-based disposition. Limits: no builds, tests, live concurrent SQL, physical data reads, or crash injection were performed. No unresolved candidate or static coverage gap remains.
Review converged after 2 round(s).
| applyBaseSnapshotValidation(rowDelta); | ||
| applyConflictDetectionFilter(rowDelta, icebergTable, commitData); | ||
| if (isSerializableIsolationLevel(icebergTable)) { | ||
| if (isSerializableIsolationLevel(icebergTable, writeOperation)) { |
Contributor
There was a problem hiding this comment.
[P1] Preserve SQL UPDATE identity before selecting isolation
Real SQL UPDATE reaches this call with writeOperation=MERGE: ExternalRowLevelUpdatePlanBuilder.buildMergePlan creates the shared merge sink, and PhysicalPlanTranslator.visitPhysicalExternalRowLevelMergeSink hardcodes WriteOperation.MERGE (PhysicalPlanTranslator.java:613-619); the write handle/context preserve it. With write.update.isolation-level=serializable (including its default) and write.merge.isolation-level=snapshot, UPDATE now reads the MERGE property and skips validateNoConflictingDataFiles, allowing a matching concurrent append that its configured isolation should reject. Conversely, UPDATE=snapshot with the default MERGE setting still rejects appends. Preserve UPDATE versus MERGE through the engine-to-connector operation and cover the SQL/planner path; the enabled UPDATE test constructs IcebergWriteContext(UPDATE) directly and misses this.
Contributor
TPC-H: Total hot run time: 28862 ms |
Contributor
TPC-DS: Total hot run time: 152102 ms |
Contributor
ClickBench: Total hot run time: 24.32 s |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
### What problem does this PR solve? Related PR: apache#69551 Problem Summary: UPDATE and SQL MERGE share the physical row-level sink and BE merge dialect. PhysicalPlanTranslator hardcoded WriteOperation.MERGE for both, so SQL UPDATE still selected write.merge.isolation-level after the connector began honoring operation-specific properties. Use the sink's existing SQL-MERGE cardinality marker to preserve UPDATE versus MERGE when constructing the connector write handle. Also validate write-provider support against the concrete operation instead of accepting any DELETE/MERGE-capable provider. Add translator assertions for both paths and include UPDATE in the mocked provider capabilities. ### Release note None ### Check List (For Author) - Test: Unit Test - PhysicalPlanTranslatorIcebergRowLevelDmlTest - ExternalRowLevelUpdatePlanBuilderTest - Behavior changed: Yes, SQL UPDATE now reads write.update.isolation-level. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all five changed files and their relevant planner, write-provider, transaction, gateway, and test paths at fb3ed36d8b29cd381a5d2efb48505bb0f0b58fbf. No new actionable findings.
The existing P1 about SQL UPDATE selecting MERGE isolation is fixed on this head. The UPDATE builder always carries requireMergeCardinalityCheck=false; SQL MERGE always carries true, independently of its actions. Planner copies preserve that distinction. The changed translator now selects UPDATE or MERGE accordingly, and the write handle, IcebergWriteContext, and transaction preserve that operation through isolation selection. The existing comment is therefore excluded from the retained blocking IDs and is not reposted.
The reviewed flow is:
SQL UPDATE -> shared logical/physical sink(cardinality=false) -> WriteOperation.UPDATE
SQL MERGE -> shared logical/physical sink(cardinality=true) -> WriteOperation.MERGE
-> plugin write handle -> Iceberg write context -> transaction RowDelta
-> corresponding write.update/write.merge.isolation-level property
Critical checkpoint conclusions:
- Goal and correctness: DELETE, UPDATE, and MERGE select their matching standard property and serializable default. Snapshot skips only conflicting-data-file validation; base-snapshot, deleted-file, conflicting-delete, and referenced-file checks remain. The behavior matches the pinned Iceberg isolation contract and RowDelta validation implementation.
- Scope and reuse: the five-file change stays focused and reuses SDK constants/parser. Shared engine code does not parse connector properties.
- Concurrency and locks: independent writers can commit between the statement read and its final metadata commit. Existing snapshot fences and transaction-manager serialization remain; no new threads, locks, lock-order edges, or IO under new locks are introduced.
- Lifecycle: statement-bound operation and existing transaction/table-lease ownership remain intact. Begin/finalize error handling, close, and statement-end cleanup paths are unchanged; no static-initialization concern is added.
- Configuration: existing Iceberg table properties are read from the transaction table without a new FE configuration or restart mechanism. The obsolete nonstandard
delete_isolation_levelkey is no longer consumed; invalid standard values fail explicitly. - Compatibility: no public SPI, storage representation, or BE sink dialect changes. UPDATE already has provider/transaction support and continues using the merge sink. Operation-sensitive Variant and nested-partition upgrade guards remain effective because SQL UPDATE writes replacement data.
- Thrift/protobuf: no definitions or field IDs change; the release-branch ID audit is inapplicable.
- Parallel paths: DELETE and shared UPDATE/MERGE RowDelta paths, data-only/delete-only MERGE, V2/V3 deletion handling, empty-source behavior, EXPLAIN, and per-handle Hive-to-Iceberg routing were checked. INSERT/OVERWRITE/REWRITE and changelog paths remain outside the changed isolation selection.
- Conditions: the cardinality-to-statement mapping is upheld by both builders and all relevant copies/settings. Admission now checks the concrete operation against the same per-handle provider used for planning; unsupported operations fail explicitly.
- Test coverage: inspected defaults, mixed operation properties, invalid-name handling, enabled concurrent-append cases for all three operations, translator identity assertions, and related builder/snapshot/serializable/V3 tests. The enabled UPDATE/MERGE append cases contain delete fragments only. The PR adds no full SQL-to-commit asymmetric-property test, single-operation-provider rejection test, or V3 snapshot/concurrent-delete matrix. These are test-strength limits; corresponding source paths were traced without an unresolved correctness candidate.
- Test results: no regression result files change. Modified JUnit expectations agree with the inspected control flow. No builds or tests were executed under this static-only contract.
- Observability and errors: existing transaction error propagation remains, invalid isolation values fail, and the admission error now identifies the operation. No additional logging or metric requirement was identified for this change.
- Persistence and failover: no Doris EditLog state or recovery protocol is added. Existing Iceberg metadata commit and FE-failure behavior remain in use.
- Writes and atomicity: replacement data and position deletes remain in one RowDelta and SDK transaction, with mandatory validation before publication. No new crash or cleanup ownership window was identified.
- FE/BE propagation: no new wire variable; operation identity stays in the engine/connector context while existing writesDataFiles/cardinality fields continue through the shared sink.
- Performance: constant-size operation selection and property lookup per RowDelta; no additional remote IO, row loop, repeated scan, or allocation growth.
- Other invariants: native partition-visible-version, MoW bitmap, BE memory/nullability, and optimizer rule-registration/property-derivation changes are inapplicable. No additional issue was substantiated.
No additional user focus was supplied. All three assigned reviewers returned NO_NEW_VALUABLE_FINDINGS after their final rechecks; main verification and the final sweep covered the entire authoritative changed-file list and all investigated risks. Review converged in round 1. Remaining limits are static-only verification and the test-strength gaps above; no unresolved finding or uncovered required static scope remains.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28289 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-DS: Total hot run time: 150685 ms |
Contributor
ClickBench: Total hot run time: 24.29 s |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69702 Problem Summary: The row-level DML admission gate now includes the rejected operation in its error message. The existing only-INSERT connector test still expects the previous generic message, so it fails even though DELETE is correctly rejected. Update the assertion to require the DELETE-specific message and align the fixture comment. Preserve the exception and distinct INSERT-message checks. ### Release note None ### Check List (For Author) - Test: Unit Test - run-fe-ut.sh --run: PhysicalPlanTranslatorAdmissionGateTest, PhysicalPlanTranslatorIcebergRowLevelDmlTest, and ExternalRowLevelUpdatePlanBuilderTest; 20 tests passed, 0 skipped - FE core Checkstyle: 0 violations - git diff --check passed - Behavior changed: No (test-only update) - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
hubgeter
marked this pull request as ready for review
November 3, 2026 09:42
hubgeter
requested review from
924060929,
CalvinKirs,
englefly,
morrySnow and
starocean999
as code owners
November 3, 2026 09:42
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 28688 ms |
Contributor
TPC-DS: Total hot run time: 151561 ms |
Contributor
TPC-H: Total hot run time: 28236 ms |
Contributor
TPC-DS: Total hot run time: 151165 ms |
Contributor
ClickBench: Total hot run time: 24.36 s |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69702 Problem Summary: Standard-only isolation selection made existing tables configured with delete_isolation_level=snapshot fall back to serializable and reject previously permitted concurrent appends. Parsing an invalid selected value only during commit also failed after backend files had been reported, when normal rollback could no longer reclaim them. Resolve the operation's standard property, legacy compatibility property, then SDK default. Parse and cache the SDK isolation enum from the opened transaction's metadata before publishing the prepared transaction or returning a backend sink. Commit uses that cached selection while retaining the other row-delta validations. ### Release note Iceberg row-level writes retain valid legacy isolation settings and reject invalid selected isolation values before backend file creation. ### Check List (For Author) - Test: Unit Test - run-fe-ut.sh --run: six targeted connector/planner classes, 237 tests discovered, 236 passed, one pre-existing unrelated disabled test - Independent Checkstyle for fe-connector-iceberg and fe-core: zero violations - Scoped git diff --check and added-lines checks passed - Behavior changed: Yes, preserve legacy-only isolation compatibility and validate the selected isolation before backend writes, fixed for the statement lifetime - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#69702
commit: ea9d174fb9d65248e0dac3c1572ff8b51a9b23fa
base: 5caaeb3e3d488fd6eb319dfd6553682e47b9be59
reviewed_at: 2026-11-04T12:29+08:00
reviewer: hubgeter
model: gpt-6.1-sol
effort: max
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28739 ms |
Contributor
TPC-DS: Total hot run time: 150967 ms |
Contributor
ClickBench: Total hot run time: 24.25 s |
924060929
approved these changes
Nov 4, 2026
924060929
left a comment
924060929
left a comment
Contributor
There was a problem hiding this comment.
Iceberg row-level DML reads a snapshot, produces files on BE, and publishes them through an FE RowDelta transaction. This change preserves UPDATE versus MERGE identity and resolves the corresponding standard isolation property before sink creation, retaining legacy-property fallback and mandatory delete/reference validations.
Independent static review of all seven changed files and relevant planner, provider, transaction, and cleanup paths found no actionable P0/P1/P2 issue. The previous UPDATE/MERGE isolation finding is fixed: the builders establish distinct cardinality flags, plan copies preserve them, and the translator now propagates the correct operation. Isolation is parsed before transaction publication and remains fixed for the statement. APPROVE for the reviewed FE scope at ea9d174.
No builds or tests were run. Live SQL/BE execution, crash injection, and a complete V3 concurrent-delete matrix were not validated. The supplied current-head snapshot reports successful P0, FE UT, compilation, and external regression checks; FE coverage is pending and required-check rules are unavailable. This is a code-review recommendation, not a merge-readiness determination.
Automated FE-focused static review (gpt-6.1-sol, high). No build or tests were run.
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: N/A
Related PR: #69551
Problem Summary:
Doris Iceberg DELETE, UPDATE, and MERGE all used the non-standard
delete_isolation_levelproperty. As a result, the standard per-operation table propertieswrite.delete.isolation-level,write.update.isolation-level, andwrite.merge.isolation-levelwere ignored. A table configured forsnapshotisolation therefore still performed serializable data-file conflict validation and rejected normal concurrent appends.This change selects the Iceberg
TablePropertieskey based onWriteOperationand parses it withIsolationLevel.fromName(). The existing delete-file, base-snapshot, conflict-filter, and referenced-data-file validations remain enabled; onlyvalidateNoConflictingDataFilesis gated on serializable isolation. Invalid isolation-level values now fail explicitly instead of being treated as snapshot isolation.The three disabled regressions from #69551 are enabled. Unit coverage also verifies per-operation key selection, serializable defaults, and invalid values.
Release note
Iceberg DELETE, UPDATE, and MERGE now honor their standard per-operation isolation-level table properties.
Check List (For Author)
Test
IcebergConnectorTransactionTestandIcebergWriteKnownBugsTestpass, including all three concurrent-append snapshot-isolation regressions.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)