Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10611 )
Change subject: IMPALA-7128 (part 1) Refactor interfaces for Db, View, Table, Partition ...................................................................... Patch Set 2: (14 comments) made an initial pass on what seems to be the non-mechanical parts of this change. http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java File fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@175 PS2, Line 175: SimpleCatalog nit: stale http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeCatalog.java File fe/src/main/java/org/apache/impala/catalog/FeCatalog.java: http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeCatalog.java@86 PS2, Line 86: HDFS nit: FS ... I realize that some of the existing classes were misnamed HDFS, which is too specific (could be local, s3, etc.). But since we're redo'ing this area, perhaps we should refer to FS instead of HDFS? http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeDb.java File fe/src/main/java/org/apache/impala/catalog/FeDb.java: http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeDb.java@27 PS2, Line 27: from the frontend nit: rephrase as "Frontend interface for interacting with a database." http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeDb.java@31 PS2, Line 31: @return Both styles exist (javadoc or not) in the code base, but would be preferable to be consistent per file (L39 method does not use @return, for example). http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeHdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/FeHdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeHdfsPartition.java@31 PS2, Line 31: HDFS FS http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeHdfsPartition.java@31 PS2, Line 31: between remove http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeHdfsPartition.java@34 PS2, Line 34: Hdfs I'd rename this to FS (couple comments about this) http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeHdfsPartition.java@61 PS2, Line 61: /** nit: add a newline. several other cases below. http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeHdfsTable.java File fe/src/main/java/org/apache/impala/catalog/FeHdfsTable.java: http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/FeHdfsTable.java@36 PS2, Line 36: Note: despite the "HDFS" nomenclature, this table type is also used for : * interacting with S3 or other Hadoop-compatible filesystems. yup! shall we adopt the more general, preferred naming now? http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@961 PS2, Line 961: Comparison nit: Comparator http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@964 PS2, Line 964: KeyValueComparator this change can introduce differences depending on how partitions are dealt with in various collections. why is this change needed in this patch? http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@563 PS2, Line 563: @Ov nit: add a line http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@581 PS2, Line 581: @O nit: add a line http://gerrit.cloudera.org:8080/#/c/10611/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2016 PS2, Line 2016: HdfsPartition.KV_COMPARATOR how did you find all places to make this type of update, Collections.sort or something broader? -- To view, visit http://gerrit.cloudera.org:8080/10611 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id55f7d2e94d81e66ce720acb6315f15a89621b31 Gerrit-Change-Number: 10611 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Thu, 07 Jun 2018 17:00:35 +0000 Gerrit-HasComments: Yes
