Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/21423 )
Change subject: IMPALA-12732: Add support for MERGE statements for Iceberg tables ...................................................................... Patch Set 6: (7 comments) Thanks for this huge work, Peti! I'm only at the beginning of reviewing, only checked the commit msg and the tests, just dumping what I have now. http://gerrit.cloudera.org:8080/#/c/21423/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21423/6//COMMIT_MSG@7 PS6, Line 7: IMPALA-12732: Add support for MERGE statements for Iceberg tables I know MERGE is a known SQL functionality but I'd find it useful to add a few sentences about the functionality itself. http://gerrit.cloudera.org:8080/#/c/21423/6//COMMIT_MSG@11 PS6, Line 11: same syntax as Hive, Spark, and Trino Would you mind adding some examples? http://gerrit.cloudera.org:8080/#/c/21423/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeModifyStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeModifyStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/21423/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeModifyStmtsTest.java@264 PS6, Line 264: AnalysisError("merge into " for me this test seems identical to the one right above. Or do I miss something? http://gerrit.cloudera.org:8080/#/c/21423/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeModifyStmtsTest.java@384 PS6, Line 384: Column permutation Is this error msg introduced by this patch or did it exist before? I found the word permutation weird here. 'Column list' could be more nature IMO http://gerrit.cloudera.org:8080/#/c/21423/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeModifyStmtsTest.java@386 PS6, Line 386: // Fewer columns in VALUES Actually, this is also a 'More columns in VALUES' case similarly to above. http://gerrit.cloudera.org:8080/#/c/21423/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeModifyStmtsTest.java@410 PS6, Line 410: AnalyzesOk("merge into functional_parquet.iceberg_partitioned target " nit: This test could go above the error cases. Also some comment about the intention could be great like 'multiple MATCHED and NOT MATCHED cases' http://gerrit.cloudera.org:8080/#/c/21423/6/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/21423/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-merge.test@115 PS6, Line 115: ---- TYPES Do you intentionally not asserting on some profile metrics from this test on? -- To view, visit http://gerrit.cloudera.org:8080/21423 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3416a79740eddc446c87f72bf1a85ed3f71af268 Gerrit-Change-Number: 21423 Gerrit-PatchSet: 6 Gerrit-Owner: Peter Rozsa <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 31 May 2024 14:23:17 +0000 Gerrit-HasComments: Yes
