Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/20677 )
Change subject: IMPALA-12313: (part 2) Limited UPDATE support for Iceberg tables ...................................................................... Patch Set 1: (11 comments) I read through once and had some quick comments. This not a proper review :-( http://gerrit.cloudera.org:8080/#/c/20677/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20677/1//COMMIT_MSG@14 PS1, Line 14: * Table has SORT BY properties Is there also a restriction that TBLPROPERTIES must specify merge-on-read? Edit: I see Impala does set this, but there may be other tables that don't have it? Also only on iceberg v2 tables? http://gerrit.cloudera.org:8080/#/c/20677/1//COMMIT_MSG@83 PS1, Line 83: Basice Spelling: "Basic" http://gerrit.cloudera.org:8080/#/c/20677/1/be/src/exec/multi-table-sink.h File be/src/exec/multi-table-sink.h: http://gerrit.cloudera.org:8080/#/c/20677/1/be/src/exec/multi-table-sink.h@32 PS1, Line 32: DataSink* CreateSink(RuntimeState* state) const override; It would be good at some point to have descriptions for all the methods. http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java File fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java: http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java@65 PS1, Line 65: additinal Spelling: additional http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/analysis/IcebergUpdateImpl.java File fe/src/main/java/org/apache/impala/analysis/IcebergUpdateImpl.java: http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/analysis/IcebergUpdateImpl.java@52 PS1, Line 52: protected This and next line could be private I think http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/analysis/IcebergUpdateImpl.java@68 PS1, Line 68: spesc Spelling: "specs" http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/analysis/IcebergUpdateImpl.java@160 PS1, Line 160: cast Nit: "Cast" http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/analysis/IcebergUpdateImpl.java@161 PS1, Line 161: table Nit: "table." http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/analysis/ModifyImpl.java File fe/src/main/java/org/apache/impala/analysis/ModifyImpl.java: http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/analysis/ModifyImpl.java@42 PS1, Line 42: abstract void buildAndValidateSelectExprs(Analyzer analyzer, Add description http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/planner/MultiDataSink.java File fe/src/main/java/org/apache/impala/planner/MultiDataSink.java: http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/planner/MultiDataSink.java@30 PS1, Line 30: public class MultiDataSink extends DataSink { Add description http://gerrit.cloudera.org:8080/#/c/20677/1/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/20677/1/fe/src/main/java/org/apache/impala/planner/Planner.java@276 PS1, Line 276: repartition Nit: "Repartition" -- To view, visit http://gerrit.cloudera.org:8080/20677 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iff0ef6075a2b6ebe130d15daa389ac1a505a7a08 Gerrit-Change-Number: 20677 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 09 Nov 2023 17:45:10 +0000 Gerrit-HasComments: Yes
