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 <t...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Jun 2018 23:26:36 +0000
Gerrit-HasComments: Yes

Reply via email to