Todd Lipcon 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:

(13 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
> It was my understanding that flowing the param name in the description is p
k, I'll english-ify it


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?
Done


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?
Done


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?
Done


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?
Done


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 potent
Done


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.
k, just moved it now instead of adding the TODO since it was easy enough (magic 
of eclipse)


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 think this looks fine:
Done


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)
I wanted to keep this a private class, and not end up with dependencies between 
the "old" code and the "new" implementation classes.

I moved the relevant util functions into FeCatalogUtil. Does that sound 
reasonable?


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
The implementation above does that (see Table.getLongParam)


http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@190
PS4, Line 190: implementations
> thats fine, but pls leave a breadcrumb for where some of the lifting comes
Done


http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@191
PS4, Line 191: stats
> yes, have a look at setTableStats in Table.java
aha! thanks for catching this.


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.
added such a TODO at class level



--
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: Todd Lipcon <t...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Jun 2018 00:42:25 +0000
Gerrit-HasComments: Yes

Reply via email to