Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21988 )
Change subject: IMPALA-13362: Implement WHEN NOT MATCHED BY SOURCE syntax for MERGE statement ...................................................................... Patch Set 2: (6 comments) The code looks great, I only found a few nits. Some additional tests could be useful. http://gerrit.cloudera.org:8080/#/c/21988/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21988/2//COMMIT_MSG@10 PS2, Line 10: statements nit: statement's http://gerrit.cloudera.org:8080/#/c/21988/2/be/src/exec/iceberg-merge-node.cc File be/src/exec/iceberg-merge-node.cc: http://gerrit.cloudera.org:8080/#/c/21988/2/be/src/exec/iceberg-merge-node.cc@211 PS2, Line 211: nit: too much indent http://gerrit.cloudera.org:8080/#/c/21988/2/be/src/exec/iceberg-merge-node.cc@227 PS2, Line 227: nit: indentation is off http://gerrit.cloudera.org:8080/#/c/21988/2/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/21988/2/fe/src/main/cup/sql-parser.cup@1063 PS2, Line 1063: if (target_or_source.equals("SOURCE")) { : parser.parseError("SOURCE", SqlParserSymbols.KW_INSERT, "UPDATE, DELETE"); : } Could you please add comment, e.g. "User wrote WHEN NOT MATCHED BY SOURCE with INSERT operation which doesn't make sense, as there is nothing to insert." http://gerrit.cloudera.org:8080/#/c/21988/2/fe/src/main/cup/sql-parser.cup@1073 PS2, Line 1073: int kw = SqlParserSymbols.KW_DELETE; : if (matched.caseType().equals(TMergeCaseType.UPDATE)) { : kw = SqlParserSymbols.KW_UPDATE; : } : parser.parseError("TARGET", kw, "INSERT"); Could you please add comment, e.g. "User wrote WHEN NOT MATCHED BY TARGET with DELETE/UPDATE operation which doesn't make sense, as there is nothing to delete/update." http://gerrit.cloudera.org:8080/#/c/21988/2/testdata/workloads/functional-query/queries/QueryTest/iceberg-merge.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-merge.test: http://gerrit.cloudera.org:8080/#/c/21988/2/testdata/workloads/functional-query/queries/QueryTest/iceberg-merge.test@360 PS2, Line 360: ==== Maybe a few additional tests could be useful, just to check we don't miss anything - target table is non-partitioned - NOT MATCHED BY SOURCE without any predicates then DELETE/UPDATE - NOT MATCHED BY SOURCE with multiple predicates then DELETE/UPDATE - MERGE statement with many cases -- To view, visit http://gerrit.cloudera.org:8080/21988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia0e0607682a616ef6ad9eccf499dc0c5c9278c5f Gerrit-Change-Number: 21988 Gerrit-PatchSet: 2 Gerrit-Owner: Peter Rozsa <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 21 Nov 2024 20:27:13 +0000 Gerrit-HasComments: Yes
