Todd Lipcon 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
> It was my understanding that flowing the param name in the description is p
k, I'll english-ify it
PS4, Line 57: full
> fully qualified?
PS4, Line 57: full
> fully qualified?
PS4, Line 77: name
> fully qualified?
PS4, Line 77: name
> fully qualified?
PS4, Line 80: validateClusteringColumns
> based on HdfsTable::addColumnsFromFieldSchemas. pls add a todo for a potent
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.
k, just moved it now instead of adding the TODO since it was easy enough (magic 
of eclipse)
File fe/src/main/java/org/apache/impala/catalog/local/
PS4, Line 78: schemaInfo;
> I think this looks fine:
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 
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)
PS4, Line 190: implementations
> thats fine, but pls leave a breadcrumb for where some of the lifting comes
PS4, Line 191: stats
> yes, have a look at setTableStats in
aha! thanks for catching this.
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
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: Todd Lipcon <>
Gerrit-Reviewer: Vuk Ercegovac <>
Gerrit-Comment-Date: Wed, 13 Jun 2018 00:42:25 +0000
Gerrit-HasComments: Yes

Reply via email to