suxiaogang223 commented on PR #70103:
URL: https://github.com/apache/doris/pull/70103#issuecomment-6652494659
Local code review completed for **94fb806bd61dcab775c8906694efd8236d2f2523**
against `branch-4.2` base **37671319cdefc44dd8e5b85a0584a4826460f03d**.
Reviewed the complete 34-file PR diff and the affected FE planning, snapshot,
catalog-construction, and BE JNI read/write call chains using the local
`code-review` skill.
**Result: PASS. No actionable findings in the reviewed diff.** This is a
review of the current revision, including the MERGE input-layout fix.
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
| --- | --- |
| Goal and proof | The shared SDK and embedded shade move to Paimon 2.1.0.
Nullable-key serialization, MERGE branch selection, integer overflow, and
pre-epoch date semantics have targeted tests. |
| Scope and reuse | Changes stay within the 4.2 SDK adaptation and affected
tests. `PaimonMergeCommand` reuses `MergeUtils.buildMergeJoin`; no
connector/plugin-layout backport is introduced. |
| Concurrency and locks | No new threads, shared mutable state, or
lock-order change. S3 mirroring runs during catalog initialization, before the
options map is published through the existing initialization path. |
| Lifecycle | No new resource owner or static-initialization dependency. SDK
catalog authorization retains its existing catalog close boundary; local
writer/commit/reader test resources are closed. |
| Configuration | No new Doris FE/BE configuration. Hadoop S3 settings are
copied into the SDK catalog options, preserving explicit Paimon values. |
| Compatibility | FE/JNI dependency management uses the same version. The
shade's embedded Paimon is aligned; embedded Hive/Iceberg class contents are
unchanged. Removal of upstream file-based privilege APIs is documented. Mixed
1.4.2/2.1.0 rolling-upgrade compatibility remains unverified and is explicitly
disclosed. |
| Thrift/Protobuf | No schema, field ID, or enum change. |
| Parallel paths | The shared S3 mapping is reached by filesystem, HMS,
JDBC, and DLF initialization. Deferred system-table checks traverse both
fallback branches. FE and JNI dependency versions change together. |
| Conditions and planner invariants | MATCHED-only MERGE uses INNER JOIN.
MERGE with INSERT uses target-left RIGHT OUTER JOIN source; the target-side
constant becomes NULL only for a missing target. Marker lookup uses ExprId, and
user binding retains the original scope. Unsupported `latest-delta` OPTIONS
fail explicitly. |
| Test coverage | Command-layout and planner tests cover both join shapes,
nullable keys, null-extension, and marker-name collisions. Regression covers
UPDATE/DELETE/INSERT, branch priority, duplicate rejection, JNI/Rust reads,
file indexes, and checked overflow. |
| Expected results | Both new golden files passed local regression
comparison. The modified snapshots golden adds exactly the SDK's nullable
`operation` and `writer_version` columns; legacy fixture snapshots lack both
fields. The full static-fixture system-table suite was not rerun locally. |
| Observability | Existing exception propagation is retained; unsupported
delta mode gives an actionable error. No new execution subsystem requiring
metrics is introduced. |
| Transactions and persistence | No EditLog/replay or failover-protocol
change. Existing cardinality checks and commit/abort paths remain in use. |
| Writes and atomicity | Nullable-key branch classification is corrected
without changing writer ownership or commit sequencing. Duplicate
matches/inserts are rejected; the transaction regression confirms a failed
oversized write publishes neither rows nor a new snapshot. |
| FE/BE variables | No new protocol variable or sending path. Serialized SDK
objects require the FE/JNI version alignment noted above. |
| Performance | The target marker is one constant projection, added only for
INSERT-capable MERGE. S3 mirroring is one configuration traversal at catalog
creation. No new per-row allocation or unbounded planning work was found in the
diff. |
| Other concerns | Environment-gated HMS/DLF tests and mixed-SDK deployment
remain outside completed local coverage. External-service CI still needs its
own successful run. |
Validation on this head in the current worktree:
- FE and JNI packaging through `build.sh`, including Checkstyle: passed.
- FE tests via `run-fe-ut.sh --run 'Paimon*Test,MergeIntoCommandTest'`:
**490 passed, 2 environment-gated skips, 0 failures**. The two command-layout
tests fail before the fix and pass afterward.
- Regression framework build: **47 unit tests passed**.
- Regression through `run-regression-test.sh`, comparing committed expected
outputs: **7 suites passed, 0 failures/skips**:
`test_paimon_nullable_key_merge`, `test_paimon_2_1_compatibility`,
`test_paimon_write_boundary`, `test_paimon_write_row_level_dml`,
`test_paimon_write_deletion_vector`, `test_paimon_write_merge_semantics`, and
the additionally checked `test_paimon_write_transaction`.
- Runtime: locally built FE/JNI using Paimon 2.1.0, this PR's successful CI
ASAN BE artifact (compile job 1092066), MinIO, and the repository's Spark
4.0.0/Paimon 1.4.2 fixtures. No new native C++ change is present.
<!-- doris-repo-review:v1:begin -->
```yaml
schema: doris-repo-review/v1
status: PASS
pr: apache/doris#70103
commit: 94fb806bd61dcab775c8906694efd8236d2f2523
base: 37671319cdefc44dd8e5b85a0584a4826460f03d
reviewed_at: 2026-11-05T07:37:50+00:00
reviewer: suxiaogang223
model: gpt-6.1-sol
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: true
```
<!-- doris-repo-review:v1:end -->
--
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]
