Vuk Ercegovac has posted comments on this change. ( )

Change subject: IMPALA-7140 (part 1). Support fetching schema info in 

Patch Set 4:

File fe/src/main/java/org/apache/impala/catalog/
PS4, Line 37: @param
lets not use javadocs.
PS4, Line 57: full
fully qualified?
PS4, Line 77: name
fully qualified?
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 
File fe/src/main/java/org/apache/impala/catalog/
PS4, Line 119: public
todo: pull it up to FeFsTable or comment at site of use.
File fe/src/main/java/org/apache/impala/catalog/local/
PS4, Line 78: schemaInfo;
I prefer the alternative style you used for these:
this.db_ = Preconditions.checkNotNull(db)
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)
PS4, Line 175: getRowCount
I saw that -1 is used as a default/unknown value in catalog.Table... needed 
here as well?
PS4, Line 190: Table
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.
PS4, Line 191: stats
is num bytes needed as well?
PS4, Line 263:     private boolean isClusteringColumn(Column c) {
duplicates the impl in Table, pls add a todo to refactor.

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Vuk Ercegovac <>
Gerrit-Comment-Date: Tue, 12 Jun 2018 23:26:36 +0000
Gerrit-HasComments: Yes

Reply via email to