Gabriel39 commented on PR #66498: URL: https://github.com/apache/doris/pull/66498#issuecomment-5215296702
I completed a follow-up review of the current head `ef5b271d`. The latest commit addresses the previous inline findings about modifying `sequence.field` and duplicate primary keys among the rows emitted by the same NOT MATCHED INSERT branch. I found the following remaining issues. ### [P1] `rowkind.field` overrides the RowKind emitted by Doris `PaimonWriteSchema.tableRow()` sets the `InternalRow` RowKind from the Doris operation column, but Paimon 1.4.2 uses `RowKindGenerator.getRowKind()` when `rowkind.field` is configured. That generator takes precedence over `row.getRowKind()`. `PaimonRowChangeCapabilities` currently does not reject or otherwise neutralize this table option. Consequently: - DELETE can be converted back into INSERT/UPDATE when the stored rowkind field contains `+I` or `+U`, so the statement succeeds without deleting the row. - A MERGE NOT MATCHED INSERT carrying `-D` in that field can become a physical DELETE. - Updating the rowkind field can turn an UPDATE into a DELETE or another operation. Please either reject row-level UPDATE/DELETE/MERGE on tables configured with `rowkind.field`, or construct a pinned writer table where the generator cannot override the operation selected by Doris. Please also add UPDATE, DELETE, and MERGE coverage for this option. ### [P1] NOT MATCHED INSERT does not detect a primary key that already exists in the target `checkInsertPrimaryKeyUniqueness()` only checks for duplicate projected keys among the INSERT rows emitted by this MERGE statement. It does not check those keys against existing target rows. For example, given an existing target row `(id=1, status='old')`, a source row `(id=1, status='new')`, and: ```sql ON t.id = s.id AND t.status = s.status ``` the source row is classified as NOT MATCHED and can emit an INSERT with `id=1`. A Paimon deduplicate table does not raise a primary-key violation; it applies upsert/sequence semantics and can silently replace or retain a row even though the SQL MERGE branch classified it as an insert. Please either validate projected INSERT keys against the target primary keys, or restrict the MERGE ON condition so that it guarantees complete primary-key matching. A regression test should cover an additional non-key ON predicate producing this collision. ### [P1] Nondeterministic INSERT key expressions are evaluated separately for validation and writing The uniqueness projection calls `generateFinalExpression()` to materialize INSERT keys, and the final sink projection calls it again for the actual output row. Expressions such as `rand()` or `uuid()` can therefore produce one key during validation and a different key during writing. The uniqueness assertion can pass while the actual sink rows collide. Please materialize the selected operation and row values once, then use the same slots for both uniqueness validation and the final sink output. ### [P2] Unmatched rows form one unbounded NULL window partition `generateTargetMatchCount()` partitions by target primary-key slots before branch filtering. In a LEFT JOIN, every NOT MATCHED row has NULL target-key slots, so all unmatched source rows enter the same window partition whenever the MERGE also contains a MATCHED clause. A normal mixed MERGE with a small number of updates and millions of inserts can therefore create a single shuffle/buffer hotspot and cause excessive spill, memory growth, or OOM. Please run the target cardinality check only for matched rows, without placing all unmatched rows into one NULL partition, and add a mostly-unmatched large-input test. ### Additional planner performance concern `Analyzer.buildAnalyzerJobs()` now runs a second full bottom-up `BindExpression` pass for every analyzed statement, although the new expressions are introduced only by sink binding. The applied-aware conditions avoid most transformations, but every plan still pays for another traversal and rule matching. Please scope the rebind to newly created sink nodes if possible, or provide planner benchmark evidence showing that the global pass does not cause a meaningful analysis-time regression. The current regression suite does not cover the four cases above. Given the silent row-operation/key correctness failures, I do not think this is ready to merge yet. -- 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]
