Repository navigation
[test](iceberg) Expand capability, transaction and snapshot coverage - #69551
Merged
Gabriel39 merged 1 commit intoOct 25, 2026
Merged
Conversation
### What problem does this PR solve? Problem Summary: Add connector semantic tests, external regression cases and an operation coverage inventory. Preserve correct, individually disabled reproductions for known issues without changing production behavior. ### Release note None ### Check List (For Author) - Test: Unit Test; full connector reactor and FE Checkstyle passed. Groovy compilation passed; SQL execution pending managed environment. - Behavior changed: No - 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.
Static review of all 12 changed files at 1014a63 found no verified new defects. The PR adds nine Java test classes, two external SQL suites and a coverage inventory; production behavior is unchanged. There are no existing inline findings to carry forward and no additional user-provided focus points.
Critical checkpoint conclusions:
- Goal and proof: the additions strengthen snapshot/schema/ref isolation, write publication, type/partition/predicate oracles, and delete/history coverage. The 45 test methods include nine explicitly disabled reproductions of pre-existing issues; those are not passing coverage or newly introduced defects.
- Scope and clarity: changes stay within Iceberg tests and their inventory, following existing JUnit 5, recording-fake and Spark/REST fixture patterns.
- Concurrency and locks: no new threads or production locking; the transaction tests model an external commit between begin and commit using separate table instances.
- Lifecycle: fixtures are per test, file-scan iterables close deterministically, begin-failure/close guards are checked, and the SQL suite restores its modified TopN threshold in finally.
- Configuration: no configuration item added. Existing scanner defaults, fixture guards and explicit merge-on-read table properties support the exercised paths.
- Compatibility: no production symbol, API, plugin surface or storage-format change; V1/V2/V3 are test inputs.
- Thrift/protobuf: no definitions or field IDs changed.
- Parallel paths: reviewed branch/tag/explicit-snapshot and metadata-table schemas; both lineage names; all predicate modes; nested mapping flags; static/dynamic/branch overwrite; UPDATE/MERGE; and V2/V3 Parquet/ORC SQL cases.
- Conditional checks: enablement, unsupported DATE/hour transforms, and stale/fresh or position/equality test branches have concrete purposes.
- Test coverage: independent scalar comparisons, SDK transforms, file sets, historical snapshots and unchanged metadata provide meaningful positive and negative oracles. The future-cutoff retention case has a real unreferenced-snapshot control. TopN eligibility and physical delete-coordinate assumptions match the current call chain.
- Expected results: no golden result files changed. Ordered SQL comparisons preserve pre-mutation coordinates and ref contents; no incorrect new expectation was established.
- Observability: no new production path requires logs or metrics; relevant assertions identify mode, type or state.
- Transactions and persistence: publication, parent ancestry, branch isolation and failed-mutation atomicity are checked; no EditLog, replay or failover logic changed.
- Data writes: test fragments match the existing BE commit protocol, including UPDATE data/delete pairing and disjoint MERGE actions. No production atomicity or crash-handling behavior changed.
- FE/BE variables: no transmitted variable or protocol field added.
- Performance: small bounded test matrices and metadata histories; no production hot-path work added.
- Other concerns: all inventory links resolve and its 84 support rows match the linked published capability matrix. Its implementation/documentation differences and partial coverage are explicitly identified.
Validation limits: this review was static only; no builds, tests, SQL, Docker fixtures or optimizer EXPLAIN plans were executed. Managed SQL execution, runtime failure/restart/failover cases and the full backend/scanner/storage matrix remain unverified, as the PR already states. Source tracing supports the TopN path, but this new suite does not itself assert its EXPLAIN shape; the neighboring metadata suite does. These are execution and coverage limits, with no unresolved introduced-defect candidate in the reviewed changes.
Review converged after 2 round(s).
Contributor
TPC-H: Total hot run time: 28366 ms |
Contributor
TPC-DS: Total hot run time: 150985 ms |
Contributor
ClickBench: Total hot run time: 24.54 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
924060929
approved these changes
Oct 24, 2026
xylaaaaa
added a commit
to xylaaaaa/doris
that referenced
this pull request
Oct 26, 2026
…merged from master ### What problem does this PR solve? Issue Number: None Related PR: apache#69392 Problem Summary: `IcebergConnectorTransaction.addCommitData` now rejects a reported file that is not under a raw location registered by `planWrite`. `IcebergWriteTransactionCoverageTest` (added on master by apache#69551) feeds commit data to a transaction directly, without `planWrite`, so 11 of its cases failed with that rejection after the merge. The test now registers `s3://bucket/events`, under which all its files live, as `IcebergConnectorTransactionTest` already does. `IcebergWriteKnownBugsTest` builds its transactions the same way; its cases are disabled, but they would fail for this unrelated reason once re-enabled, so it registers the location too. ### Release note None ### Check List (For Author) - Test: Unit Test (FE UT in CI) - Behavior changed: No - Does this need documentation: No
This was referenced Oct 26, 2026
4 tasks done
hubgeter
added a commit
to hubgeter/doris
that referenced
this pull request
Oct 28, 2026
### 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
Gabriel39
pushed a commit
that referenced
this pull request
Oct 28, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: #69551 Problem Summary: `expire_snapshots` built its delete-file kind map from a table handle that could predate delete files committed by another writer. Iceberg refreshes table metadata during expiration, so the callback could receive an unmapped position or equality delete file and count it as a data file. Refresh the table before building the file-kind map so classification and expiration operate on the same metadata generation. Add regression coverage for position and equality delete files committed after the table was loaded. Before the fix, both cases returned `[1, 0, 0, 2, 3, 0]`. After the fix, position deletes return `[0, 1, 0, 2, 3, 0]` and equality deletes return `[0, 0, 1, 2, 3, 0]`. ### Release note Fix `expire_snapshots` misreporting position and equality delete files as data files after concurrent metadata commits. ### Check List (For Author) - Test - [ ] Regression test - [x] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason Validated on latest `apache/master`: `IcebergExpireSnapshotsActionTest`, 15 tests passed. The full `fe-connector-iceberg` suite previously passed 1739 tests with 14 skipped. - Behavior changed: - [ ] No. - [x] Yes. The reported position/equality delete counters are corrected. - Does this need documentation? - [x] No. - [ ] Yes. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Oct 29, 2026
Apply the outstanding changes in master merge order: apache#69545 7d16729 apache#69551 7224a2d apache#69612 61d1aef apache#69615 8d2bd1e apache#69650 3dc7341 apache#69651 c00ecb1 apache#69646 e493c61 apache#69668 7865646 apache#69614 already exists on branch-4.2 through apache#69644. Adapt datasource APIs and tests to the release branch, retaining its existing scanner dispatch and statement-bound metadata contracts.
yiguolei
pushed a commit
that referenced
this pull request
Oct 30, 2026
### What problem does this PR solve? Backport the requested external-data and FileScannerV2 changes to `branch-4.2` in their actual master merge order: | Order | Master PR | Change | Backport | | --- | --- | --- | --- | | 1 | #69545 | Preserve Paimon schema options after field renames | Included | | 2 | #69551 | Expand Iceberg capability, transaction and snapshot tests | Included with 4.2 API adaptations | | 3 | #69614 | Elasticsearch HTTP scans in FileScannerV2 | Already present through #69644 (`2104b7509cda`); not applied twice | | 4 | #69612 | Check catalog visibility before SHOW DATABASES accesses metadata | Included | | 5 | #69615 | Publish Iceberg Parquet split offsets | Included | | 6 | #69650 | Preserve ES compound residuals and expand scanner coverage | Included | | 7 | #69651 | Accept Elasticsearch 9.x during node discovery | Included | | 8 | #69646 | Prioritize Parquet V2 row groups using TopN bounds | Included | | 9 | #69668 | Arrow loads and schema inference in FileScannerV2 | Included | The eight outstanding changes are combined in one commit. This branch retains the 4.2 FE datasource APIs instead of importing master's connector-module refactor. ### Backport adaptations - Port Paimon schema restoration, catalog-option provenance, fallback handling and tests to the existing table-copy/serialization lifecycle. Keep statement-bound fields when reader options are applied after a schema-only rename. - Exercise Iceberg's existing metadata and transaction entry points with real in-memory tables and retained-generation fixtures. Preserve the nine disabled known-bug reproductions; omit two connector-only lifecycle tests. Expiration checks verify live data-file sets and exact delete-file counters; 4.2's separate manifest-name/data-counter issue remains unchanged. The delete/history SQL suite checks visible rows and eager/late-materialization parity because 4.2 does not expose master's `_file`/`_pos` columns. The inventory labels upstream-only evidence explicitly. - Port ES residual tracking to the legacy planner and add compound OR/NOT, docvalue eligibility and Thrift-context tests there. Use Java 8-compatible HTTP APIs for opt-in ES 9 tests and the branch's supported node-address fixtures. - Keep the existing full-schema Iceberg metadata-table scan, which already supplies the file-size dependency for split offsets. Adapt the external write suite to verify offsets without requiring master-only distribution plans or golden output. - Use the existing Parquet statistics interface and physical row-location representation. Keep the TopN scheduling and pruning invariants. - Include the shared Arrow array normalizer required by the new reader, taken from the same master revision. Preserve 4.2 Lance/Avro dispatch; omit the schema-inference test for the unavailable Hive OpenCSV parser. ### Validation - FE production and test sources compile with the branch's Java 8 target. Across the targeted runs and focused reruns, 20 test classes report 181 discovered, 171 passed, zero failures/errors, and 10 skipped: nine intentionally disabled known-bug reproductions plus the opt-in live ES 9 class. - FE Checkstyle: passed with zero violations. - Clang compiler syntax checks: all 23 affected C++ translation units passed against the current sources and freshly generated Thrift, using cached third-party/generated build dependencies. Focused BE runtime validation is listed below. - clang-format 16: all 30 affected C++ source/header files passed. - Groovy 4.0.19 syntax and AST conversion: all seven changed suites passed. All 105 local inventory links resolve. - Complete staged-diff self-review, whitespace/conflict checks, and sensitive-information checks passed. A full BE rebuild and SQL/external-service regressions were not run locally. Live ES 9 tests require their opt-in endpoint. CI is requested for the combined backport. ### Release note Fix Paimon reads after field renames, external-catalog SHOW DATABASES authorization, and ES compound residual filtering. Support ES 9.x node discovery, publish Iceberg Parquet split offsets, improve Parquet V2 TopN row-group scheduling, and use FileScannerV2 for Arrow loads and text/JSON schema inference. ### Check List (For Author) - Test - [x] Unit Test - [x] Regression test additions and syntax checks; SQL execution pending CI - Behavior changed: - [x] Yes. Backport the changes listed above. - Does this need documentation? - [x] No. Existing interfaces and settings are retained; the test coverage inventory is included. ### Follow-up fixes - Set the Thrift `enable_file_scanner_v2` default to `true`, matching the session default. Preserve explicit `false` for legacy query scans and extend the existing load-selection test. - Keep the selected Paimon snapshot and schema together during relation projection. Historical sequence/aggregation options must not be combined with the latest field names. Read-only principals can resolve historical selectors without INSERT permission. - Read legacy append schemas with missing or empty bucket keys without bucket pruning, preserving every historical file. The regression covers both unaware and multiple fixed buckets. Follow-up validation: 169 targeted Paimon FE tests passed with zero failures/errors/skips. FE production compilation and Checkstyle passed. Both changed C++ test translation units passed compiler syntax checks and clang-format 16. Regenerated Java/C++ defaults are enabled; a Java 8-targeted protocol check verifies constructor/clear defaults, an omitted wire field, and an explicit-false round trip. Complete BE CI and live external SQL suites remain pending. The two new reproductions first failed with the same missing-bucket-key and renamed-sequence-field errors as CI. A separate read-only-principal reproduction also failed before the permission correction. Complete external regression execution remains pending CI. ### BE unit-test compatibility - Complete the native Arrow Variant input dependency for `branch-4.2`: accept binary storage and the Parquet Variant extension, validate metadata and payloads before appending, and retain owned data after Arrow buffers are released. Allow that ingestion path through the load reader while preserving query-format restrictions and source-default validation. - Preserve Variant V2 markers in the Arrow test fixtures, including nested types. Verify timestamp input against this branch's SerDe for positive, negative, and second-boundary values. Match the existing DATE range diagnostic in the TopN pruning test. - Add tests for sliced and empty reads, invalid ranges and carriers, malformed metadata/payloads, NULL children, atomic failure, buffer ownership, and the query capability boundary. Validation: all 13 previously failing BE tests passed. The expanded run passed all 309 tests across nine suites, including Arrow/JSON loads, load validation, Iceberg, table mapping, and Variant input/output. The focused executable rebuilds the affected readers and SerDe against cached 4.2 build dependencies; this is not a complete BE rebuild. All seven changed C++ translation units passed compiler checks with current headers/generated Thrift, and all 33 affected C/C++ files passed clang-format 16. The complete staged diff, whitespace, and sensitive-information checks passed. Full CI is requested with `run buildall` and `/review`.
Gabriel39
pushed a commit
that referenced
this pull request
Nov 4, 2026
### 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_level` property. As a result, the standard per-operation table properties `write.delete.isolation-level`, `write.update.isolation-level`, and `write.merge.isolation-level` were ignored. A table configured for `snapshot` isolation therefore still performed serializable data-file conflict validation and rejected normal concurrent appends. This change selects the Iceberg `TableProperties` key based on `WriteOperation` and parses it with `IsolationLevel.fromName()`. The existing delete-file, base-snapshot, conflict-filter, and referenced-data-file validations remain enabled; only `validateNoConflictingDataFiles` is 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 - [ ] Regression test - [x] Unit Test: `IcebergConnectorTransactionTest` and `IcebergWriteKnownBugsTest` pass, including all three concurrent-append snapshot-isolation regressions. - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason - Behavior changed: - [ ] No. - [x] Yes. Snapshot isolation now permits concurrent data-file appends while retaining the other row-delta validations. - Does this need documentation? - [x] No. - [ ] Yes. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
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?
Iceberg has broad regression coverage, but feature names alone miss state transitions and weak oracles. For example, expiring fresh snapshots with a cutoff in 2020 never exercises ref retention.
Add 45 connector unit tests, two external regression suites, and an inventory mapping the published 84 operations plus read, version, model, lineage, type, scanner and storage dimensions to existing tests and remaining gaps. The tests cover snapshot/schema/ref isolation, atomic writes and failure paths, nested type metadata, exact predicate semantics, partition serialization, actual ref retention, and file positions across deletes and history.
Nine individually disabled reproductions preserve correct assertions for five confirmed issue families: DECIMAL-to-DOUBLE pruning, lineage-name collisions during a V2-to-V3 upgrade, ignored per-operation isolation properties, mixed MERGE affected-row counts, and expiration counters built from stale metadata. These were run and observed failing before disabling. Production behavior is unchanged.
Release note
None
Validation
mvn -f fe/pom.xml -pl :fe-connector-iceberg -am test -DfailIfNoTests=false -Dmaven.build.cache.enabled=falsewith the matching Thrift 0.24.0 compiler: BUILD SUCCESS, including FE Checkstyle.Check List (For Author)