Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20477 )
Change subject: IMPALA-12313: (part 1) Refactor modify statements ...................................................................... Patch Set 1: (15 comments) Thanks everyone for the comments! http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/IcebergDeleteImpl.java File fe/src/main/java/org/apache/impala/analysis/IcebergDeleteImpl.java: http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/IcebergDeleteImpl.java@20 PS1, Line 20: import java.util.List; > Unused import Done http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/IcebergModifyImpl.java File fe/src/main/java/org/apache/impala/analysis/IcebergModifyImpl.java: http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/IcebergModifyImpl.java@21 PS1, Line 21: import org.apache.impala.catalog.FeKuduTable; > Unused import Done http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/IcebergModifyImpl.java@84 PS1, Line 84: Set<SlotId> uniqueSlots, Set<SlotId> keySlots, Map<String, Integer> colIndexMap) > line too long (108 > 90) Done http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/KuduDeleteImpl.java File fe/src/main/java/org/apache/impala/analysis/KuduDeleteImpl.java: http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/KuduDeleteImpl.java@20 PS1, Line 20: import java.util.List; > Unused import Done http://gerrit.cloudera.org:8080/#/c/20477/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/20477/1/fe/src/main/java/org/apache/impala/analysis/KuduModifyImpl.java@55 PS1, Line 55: List<Integer> referencedColumns, Set<SlotId> uniqueSlots, Set<SlotId> keySlots, > line too long (107 > 90) Done http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/KuduUpdateImpl.java File fe/src/main/java/org/apache/impala/analysis/KuduUpdateImpl.java: http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/KuduUpdateImpl.java@20 PS1, Line 20: import java.util.List; > Unused import Done http://gerrit.cloudera.org:8080/#/c/20477/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/20477/1/fe/src/main/java/org/apache/impala/analysis/ModifyImpl.java@71 PS1, Line 71: correspond > Nit: corresponding Done http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/ModifyImpl.java@104 PS1, Line 104: creates > Nit: "created by" Modified to: "This only creates the sourceStmt_ once, ..." http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/ModifyImpl.java@131 PS1, Line 131: out parameter referencedColumns > I'm guessing this is now the field "referencedColumns_" Done http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java File fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java: http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@26 PS1, Line 26: import java.util.Map; > Unused imports Done http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@63 PS1, Line 63: * > line has trailing whitespace Done http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@122 PS1, Line 122: * clause. It also identifies the target table. Raises errors for unsupported tables > typo: table Done http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@212 PS1, Line 212: public QueryStmt getQueryStmt() { return modifyImpl_.getSourceStmt(); } > nit: could be getSourceStmt, or ModifyImpl's getSourceStmt could be getQuer Renamed ModifyImpl's getSourceStmt because it is a less invasive change. http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/UpdateStmt.java File fe/src/main/java/org/apache/impala/analysis/UpdateStmt.java: http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/UpdateStmt.java@29 PS1, Line 29: import org.apache.impala.planner.TableSink; > org.apache.impala.planner.TableSink, org.apache.impala.thrift.TSortingOrder Done http://gerrit.cloudera.org:8080/#/c/20477/1/fe/src/main/java/org/apache/impala/analysis/UpdateStmt.java@78 PS1, Line 78: private void createModifyImpl() { > createModifyImpl could be extracted to ModifyStmt as an abstract method Done -- To view, visit http://gerrit.cloudera.org:8080/20477 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If15f64944f2e23064b7112ad5930abc775dd65ec Gerrit-Change-Number: 20477 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-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 19 Sep 2023 09:51:44 +0000 Gerrit-HasComments: Yes
