Zoltan Borok-Nagy 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 2: (12 comments) Thanks for the comments. I'll add the remaining tests next week. 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? I listed the limitations that I'm planning to resolve in the scope of IMPALA-12313. I added an extra sentence to make it clear. http://gerrit.cloudera.org:8080/#/c/20677/1//COMMIT_MSG@83 PS1, Line 83: ting: > Spelling: "Basic" Done 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: class MultiTableSinkConfig : public DataSinkConfig { > It would be good at some point to have descriptions for all the methods. Added comments. 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: additiona > Spelling: additional Done 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: private L > This and next line could be private I think Done http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/analysis/IcebergUpdateImpl.java@68 PS1, Line 68: specs > Spelling: "specs" Done http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/analysis/IcebergUpdateImpl.java@160 PS1, Line 160: targ > Nit: "Cast" Done http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/analysis/IcebergUpdateImpl.java@161 PS1, Line 161: n> co > Nit: "table." Done http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/analysis/KuduModifyImpl.java File fe/src/main/java/org/apache/impala/analysis/KuduModifyImpl.java: http://gerrit.cloudera.org:8080/#/c/20677/1/fe/src/main/java/org/apache/impala/analysis/KuduModifyImpl.java@167 PS1, Line 167: selectList.add(new S > 'sortExprs_' is defined in ModifyImpl.java and has the following comment: Thanks for catching this. It didn't cause a problem because * addPartitioningColumn() was actually unused => removed it * addKeyColumn was always invoked with isSortingColumn=false => removed this parameter. * Planner.createPreDmlSort() ignored the partition exprs, only looked at sort exprs => now it uses both to behave like the commit message suggests, and in IcebergDeleteImpl and IcebergUpdateImpl I don't add the partition exprs to 'sortExprs_' anymore. 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: /** > Add description Done 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: /** > Add description Done 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" Done -- 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: 2 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Andrew Sherman <[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: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 17 Nov 2023 15:12:25 +0000 Gerrit-HasComments: Yes
