924060929 commented on PR #67395: URL: https://github.com/apache/doris/pull/67395#issuecomment-6055959362
FE framework review against head `b1fc0c59e033d0a1632288385358ae9b301c856f`. The write path is command → sink binding (column layout/defaults/casts) → expression and physical-property optimization → translation → execution/transaction completion. Paimon needs connector-specific routing and correct MERGE branch evaluation, but several changes here also affect the generic path. Please separate the framework fixes from the Paimon implementation, or document and validate their behavior independently. 1. **Preserve the short-circuit boundary after expression rewrites.** In [ExpressionBottomUpRewriter](https://github.com/apache/doris/blob/b1fc0c59e033d0a1632288385358ae9b301c856f/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/ExpressionBottomUpRewriter.java#L103-L108), this PR removes the `RequiresShortCircuitEvaluation` check before rewriting the returned expression's children. Continuing into a selected ordinary branch is correct, but a changed expression can still retain a short-circuit boundary. `rewriteChildren()` does not enforce that boundary itself. Please retain the check or explain the invariant that makes it unnecessary, and test a rewrite that returns another guarded expression with an invalid cast in an inactive branch. This is a framework-invariant concern; I have not established a currently reachable SQL reproducer for wrong results. 2. **Use the short-circuit contract consistently.** The common-subexpression collector replaces the interface check with `visitShortCircuitIf`, and replacement/condition rewriting also contain concrete-class handling. Protecting the replacement path is useful, but an expression implementing `RequiresShortCircuitEvaluation` should receive the generic traversal/extraction protections without needing changes in each visitor. Only `ShortCircuitIf` currently implements the interface, so this is an abstraction concern rather than evidence of another existing expression regressing. 3. **Bind the write layout once and make later stages consume it.** [PhysicalConnectorTableSink](https://github.com/apache/doris/blob/b1fc0c59e033d0a1632288385358ae9b301c856f/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalConnectorTableSink.java#L536-L545) now shares a lazily initialized context across plan copies. Reusing the same connector resolution for property derivation and translation is a reasonable goal. However, binding already decides the output layout using connector traits, while this context resolves those traits again later. Prefer resolving the write requirements at a defined planning stage, carrying the bound layout and immutable requirements in the plan, and keeping connector service objects in the statement planning context. Please clarify why lazy resolution inside physical-property derivation is the appropriate ownership boundary. 4. **Keep static partition values in one representation.** [PluginDrivenInsertCommandContext](https://github.com/apache/doris/blob/b1fc0c59e033d0a1632288385358ae9b301c856f/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/PluginDrivenInsertCommandContext.java#L45-L87) represents values as a string map plus a separate set of SQL-NULL keys. Distinguishing SQL NULL from the string `'NULL'` is necessary, and extracting the duplicated INSERT/OVERWRITE conversion is reasonable. A neutral typed partition-value representation would avoid splitting one value across two structures and requiring every consumer to reconstruct its meaning. This is a design concern, not a claim that the current Paimon consumer ignores the NULL set. 5. **The BindSink cast relocation is justified.** Static partition values now enter `getColumnToOutput()` before omitted-column validation and are cast there. This fixes the false “column has no default value” error for a supplied non-null static partition column. The cast is retained; I would keep this fix and cover the non-null/type-conversion cases independently of Paimon. 6. **Fix the affected-row contract in the new Paimon transaction.** `ConnectorTransaction.getUpdateCnt()` defaults to zero, and `PluginDrivenInsertExecutor.doBeforeCommit()` overwrites the coordinator's row count for any nonnegative value. `PaimonConnectorTransaction` does not override it, so successful writes report zero affected rows. Return the actual count, or `-1` to preserve the coordinator count, and add a test for the client-visible INSERT result. This is a concrete P2 behavior issue, separate from the design concerns above. This is a static FE review; I did not run the build or regression tests. In particular, the short-circuit traversal concern should not be presented as a reproduced P1 until a reachable trigger is demonstrated. -- 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]
