Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10630 )
Change subject: IMPALA-7140 (part 1). Support fetching schema info in LocalCatalog ...................................................................... Patch Set 4: (12 comments) http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java File fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java: http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java@37 PS4, Line 37: @param lets not use javadocs. http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java@57 PS4, Line 57: full fully qualified? http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java@77 PS4, Line 77: name fully qualified? http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java@80 PS4, Line 80: validateClusteringColumns based on HdfsTable::addColumnsFromFieldSchemas. pls add a todo for a potential refactor there (would be nice not to duplicate these types of exception strings). http://gerrit.cloudera.org:8080/#/c/10630/4/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/10630/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@119 PS4, Line 119: public todo: pull it up to FeFsTable or comment at site of use. http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java: http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@78 PS4, Line 78: schemaInfo; I prefer the alternative style you used for these: this.db_ = Preconditions.checkNotNull(db) http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@175 PS4, Line 175: HdfsTable Table? (so that the child class is not used) In either case, this is an odd dep. Seems like SchemaInfo can be the class that knows specifically about HMS, so the detail about what param to pull out would go there explicitly (then replace that method of Table) http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@175 PS4, Line 175: getRowCount I saw that -1 is used as a default/unknown value in catalog.Table... needed here as well? http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@190 PS4, Line 190: Table FeTable? http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@190 PS4, Line 190: implementations Got confused about whether this is supposed to factor existing code in Table. Looks like it partially does-- pls clarify whether the intent is to factor or leave the duplication. http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@191 PS4, Line 191: stats is num bytes needed as well? http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@263 PS4, Line 263: private boolean isClusteringColumn(Column c) { duplicates the impl in Table, pls add a todo to refactor. -- To view, visit http://gerrit.cloudera.org:8080/10630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I640f27e36198955e057da62a3ce25a858406e496 Gerrit-Change-Number: 10630 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 12 Jun 2018 23:26:36 +0000 Gerrit-HasComments: Yes
