Zoltan Borok-Nagy 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 13: Code-Review+1 (9 comments) Left a few comments, but I think we are close to the finish line :) http://gerrit.cloudera.org:8080/#/c/21423/11/be/src/exec/iceberg-merge-node.h File be/src/exec/iceberg-merge-node.h: http://gerrit.cloudera.org:8080/#/c/21423/11/be/src/exec/iceberg-merge-node.h@110 PS11, Line 110: ergeCase*> matched_cases_; > It would require the null checks to be moved into EvaluateCases, as the res I meant we could just save 'Tuple* previous_row_target_tuple' instead of 'last_row_'. And IsDuplicateRow() could still have 'TupleRow* actual_row' as its parameter. Duplicate check could be a bit faster and simpler, but feel free to ignore this comment if you think it doesn't make too much sense. http://gerrit.cloudera.org:8080/#/c/21423/11/be/src/exec/iceberg-merge-node.h@134 PS11, Line 134: > The other way to initialize non-trivial object is a separate line like 'con Ack http://gerrit.cloudera.org:8080/#/c/21423/13/be/src/exec/iceberg-merge-node.cc File be/src/exec/iceberg-merge-node.cc: http://gerrit.cloudera.org:8080/#/c/21423/13/be/src/exec/iceberg-merge-node.cc@251 PS13, Line 251: int target_tuple_idx Is 'target_tuple_idx' needed? It's always target_tuple_idx_. http://gerrit.cloudera.org:8080/#/c/21423/13/be/src/exec/iceberg-merge-node.cc@251 PS13, Line 251: TupleRow* previous_row Is 'previous_row' needed? It's always last_row_. http://gerrit.cloudera.org:8080/#/c/21423/13/fe/src/main/java/org/apache/impala/analysis/DmlStatementBase.java File fe/src/main/java/org/apache/impala/analysis/DmlStatementBase.java: http://gerrit.cloudera.org:8080/#/c/21423/13/fe/src/main/java/org/apache/impala/analysis/DmlStatementBase.java@134 PS13, Line 134: } nit: missing empty line after this http://gerrit.cloudera.org:8080/#/c/21423/11/fe/src/main/java/org/apache/impala/analysis/IcebergMergeImpl.java File fe/src/main/java/org/apache/impala/analysis/IcebergMergeImpl.java: http://gerrit.cloudera.org:8080/#/c/21423/11/fe/src/main/java/org/apache/impala/analysis/IcebergMergeImpl.java@258 PS11, Line 258: > Unfortunately, it's required, if it's a simple slash (/), then in terminate I see, since this SQL statement generates a parse error anyway, you could just write "* /", or use /// instead of /** .. */ for the comments. http://gerrit.cloudera.org:8080/#/c/21423/13/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: http://gerrit.cloudera.org:8080/#/c/21423/13/fe/src/main/java/org/apache/impala/planner/Planner.java@142 PS13, Line 142: { nit: missing space http://gerrit.cloudera.org:8080/#/c/21423/13/testdata/workloads/functional-query/queries/QueryTest/iceberg-merge-long.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-merge-long.test: http://gerrit.cloudera.org:8080/#/c/21423/13/testdata/workloads/functional-query/queries/QueryTest/iceberg-merge-long.test@535 PS13, Line 535: row_regex:.*[0-9]+ Would it be possible to check actual results? I see currently you are using random, but maybe we could switch to dummy data. http://gerrit.cloudera.org:8080/#/c/21423/13/tests/stress/test_merge_stress.py File tests/stress/test_merge_stress.py: http://gerrit.cloudera.org:8080/#/c/21423/13/tests/stress/test_merge_stress.py@100 PS13, Line 100: new_total new_total seems unnecessary -- 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: 13 Gerrit-Owner: Peter Rozsa <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[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, 16 Aug 2024 10:31:48 +0000 Gerrit-HasComments: Yes
