morningman opened a new pull request, #68858:
URL: https://github.com/apache/doris/pull/68858
### What problem does this PR solve?
Issue Number: None
Related PR: #65868, #66498, #67395
Original author: @suxiaogang223 (the cases follow the tests of branch-4.1
#65868 and #66498, rewritten for master's Paimon connector; the commit carries
Co-authored-by)
Problem Summary:
**In short.** master #67395 forward-ported the branch-4.1 Paimon write path,
and comparing it with branch-4.2 found the write behavior in place on master.
What did not come along is most of the 4.x tests. `PaimonConnectorTransaction`,
which commits every Paimon write on the FE, has no test at all, and several
other rules are unpinned. This PR ports those tests. It changes no production
code.
**Background.** #67395 carries the branch-4.1 Paimon write PRs #65868,
#66325, #66498, #66810, #66612, #66990, #67023, #67085 and #68167, rebuilt on
the connector write framework of #68320. The BE tests and the BE-side Java
writer tests came over unchanged. The FE tests did not: they target fe-core
classes that master replaced with the Paimon connector and the generic write
framework (`PaimonTransaction`, `PhysicalPaimonTableSink`, `PaimonMetadataOps`).
**The problem, and what it cost.** Nothing fails on master if any of the
following regresses:
- **FE commit.** A commit whose response is lost is retried once, then
reconciled against the snapshots Paimon published for this commit user and
transaction id. If no snapshot is found, the transaction ends in
OUTCOME_UNKNOWN and its files are never aborted, because the commit may still
land. A failure before the commit started stays abortable. A close failure
after a successful commit does not change the outcome. Payloads that a BE
reports twice are de-duplicated by their exact bytes, not by their hash. The
commit user carries the Doris cluster id. A broken rule here either loses
committed data (aborting files of a commit that landed) or leaks it (keeping
files of a commit that never landed).
- **Single writer.** Some bucket modes must be written by a single writer:
fixed-bucket append tables that are not write-only (their writers also
compact), primary-key fixed-bucket tables, and dynamic or cross-partition
dynamic buckets. Two writers on one bucket corrupt it.
- **MODIFY COLUMN.** Doris shows a Paimon TIMESTAMP(9) as DATETIMEV2(6). A
MODIFY COLUMN that keeps that type (for example, to change the comment) must
not narrow the remote column to TIMESTAMP(6).
- **Short-circuit evaluation.** MERGE INTO labels its branches with
`ShortCircuitIf`. `NestedCaseWhenCondToLiteral` and `NullSafeEqualToEqual` must
keep the function when they rewrite its condition, or the branch values lose
their mandatory short-circuit evaluation.
- **Catalog flavors.** master writes through HMS and REST Paimon catalogs,
but only filesystem and JDBC catalog writes have end-to-end cases.
- **MTMV.** The MTMV null-partition case checks that four partition names
are present, but not that there are exactly four.
**How this PR fixes it.** Tests only:
| Test | What it pins |
|---|---|
| `PaimonConnectorTransactionTest` (new, 7) | the six branch-4.x commit
cases above, plus payloads reported twice committing once |
| `PaimonWritePlanProviderTest` (+2) | which bucket modes get a single
writer (GATHER) and which do not |
| `PaimonConnectorMetadataColumnEvolutionTest` (+1) | MODIFY COLUMN keeps
the remote TIMESTAMP(9) |
| `NestedCaseWhenCondToLiteralTest`, `NullSafeEqualToEqualTest` (+1 each) |
both rewrites keep a ShortCircuitIf |
| `test_paimon_hms_catalog_write`, `test_paimon_rest_catalog_write` (p2,
new) | append and primary-key writes through HMS and REST catalogs |
| `test_paimon_mtmv` | the MV gets exactly four null-partition variants |
The transaction test needs no mocking framework, which this module does not
use. Its table is a real on-disk Paimon table, so reconciliation reads the
snapshot that the first attempt really published; only the committer the table
hands out is scripted. The single-writer fixtures use a DATE bucket key. master
cannot route that type natively, so these fixtures reach the single-writer
decision instead of native bucket routing.
**Results.** Each run below made the listed changes to the production code
locally and ran the affected test classes:
| Changes to production code | Result |
|---|---|
| no reconciliation after two failed attempts; payloads de-duplicated by
hash | `publishedSnapshotReconcilesAsCommitted` and
`commitPayloadDedupUsesExactContent` fail, the other 5 pass |
| OUTCOME_UNKNOWN aborts on rollback; a failed committer open is retried |
`missingSnapshotLeavesOutcomeUnknown` and `preCommitFailureRemainsAbortable`
fail, the other 5 pass |
| a close failure after commit counts as a commit failure |
`closeFailureAfterCommitDoesNotChangeOutcome` fails (the commit is opened
twice), the other 6 pass |
| no single-writer rule for fixed buckets; MODIFY COLUMN always converts the
type | both single-writer cases and
`modifyColumnKeepsRemoteTimestampPrecisionBehindLossyProjection` fail, the 13
existing cases pass |
| the two rewrites rebuild a plain `If` | both `testPreserveShortCircuitIf`
fail, the 14 existing cases pass |
The comparison also found behavior differences, which are handled in
separate PRs: the Paimon metadata cache, static partition overwrite values,
Java short time zone names, and the JNI read batch size.
**Classes, and how they call each other.**
```
PluginDrivenInsertExecutor
└─ PaimonConnectorTransaction
├─ addCommitData(bytes) BE final report: TPaimonCommitMessage, kept
once per exact payload
├─ commit() newCommit(commitUser).filterAndCommit,
retried once;
│ then
snapshotManager().findSnapshotsForIdentifiers(commitUser, [txnId])
└─ rollback() COMMITTED / COMMITTING / OUTCOME_UNKNOWN:
keep files; PREPARED: abort
PaimonWritePlanProvider.getWriteDistribution
native fixed-bucket route ─> BUCKET_UNAWARE: any ─> single writer: GATHER
─> primary key: hash ─> unpartitioned
```
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] Regression test
- The p2 cases need the p2 environment and were not run. Their .out
files are the ones branch-4.2 generated; the same statements on a local
filesystem Paimon catalog on master return the same rows.
- The `test_paimon_mtmv` change needs the Paimon docker fixture and
was not run locally.
- [x] Unit Test
- `PaimonConnectorTransactionTest` (7),
`PaimonWritePlanProviderTest` (9), `PaimonConnectorMetadataColumnEvolutionTest`
(7), `NestedCaseWhenCondToLiteralTest` (5), `NullSafeEqualToEqualTest` (11)
pass locally; mutation results above.
- Checkstyle: 0 violations in `fe-connector-paimon` and `fe-core`.
- [ ] 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 <!-- Add your reason? -->
- Behavior changed:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/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 -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
