suxiaogang223 commented on PR #66498: URL: https://github.com/apache/doris/pull/66498#issuecomment-5306304686
Follow-up on the two latest review comments (the code changes below are prepared locally and will be included in the next push): ### Preserve forced short-circuit semantics across `IF` rewrites Confirmed. `ConditionRewrite.visitIf()` and `NestedCaseWhenCondToLiteral.visitIf()` now rebuild a changed node through the original `ifExpr.withChildren(...)` instead of `new If(...)`. This preserves the concrete `ShortCircuitIf` type and its `AlwaysShortCircuit` marker without adding a Paimon-specific check to the generic rewrite rules. The Paimon MERGE regression now uses `s.id <=> 1` under `short_circuit_evaluation=false`, so `NullSafeEqualToEqual` actually rewrites a descendant of the generated `ShortCircuitIf`. The later failing predicate and the inactive assignment remain in the case, verifying that they are not evaluated. FE compilation and Java checkstyle pass. ### UPDATE / MERGE UPDATE concurrency fence I traced this against the Paimon 1.4.2 implementation used by Doris. This is a limitation of the ordinary Paimon primary-key upsert concurrency model, rather than a missing Doris commit option: - Flink batch UPDATE emits a complete `UPDATE_AFTER` row, and `applyRowLevelUpdate(..., context)` does not use the scan context for conflict validation. - Paimon's MERGE action explicitly documents that it uses upsert semantics rather than update semantics. - `commit.strict-mode.last-safe-snapshot` does not reject APPEND-versus-APPEND commits. - `appendCommitCheckConflict(true)` validates file/bucket/LSM structural conflicts, not logical same-primary-key changes in concurrent level-0 appends. - `rowIdCheckConflict(snapshot)` is for the data-evolution/row-id path and is not available as a fence for ordinary primary-key UPDATE. Consequently, a Doris-side latest-snapshot pre-check would have a TOCTOU window, and a Doris table lock would not coordinate Flink or other external Paimon writers. Rejecting `sequence.field` would only hide one manifestation and would not prevent the non-sequence lost-update case. A correct fence requires an upstream Paimon atomic primitive that validates the scan snapshot or affected primary keys during commit, followed by retrying the entire statement from a fresh scan. A commit-only retry is indeed insufficient. This should be tracked as an upstream Paimon capability plus a Doris follow-up; this PR follows Paimon's native upsert/sequence concurrency semantics and should not add a fence that cannot provide the promised guarantee. If row-level conflict detection is considered a prerequisite for exposing SQL UPDATE, the safe alternative is to defer UPDATE/MERGE UPDATE until that upstream primitive exists. -- 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]
