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: (4 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. why not? I agree with not slavish adherence to Javadoc when the parameters are clear, but in this case it's useful to know "for use in exception messages" because it's not really obvious why this method would take a tableName. 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: the problem is I needed to use tblName on line 75, so I need to checkNotNull for tblName ahead of that. I could move just that one up, and use the checkNotNull on schemaInfo and db here, but then it's less consistent which I thought might look odd. Another option is to not bother doing checkNotNull on tblName and just let the tblName.toLowerCase() throw NPE if it's null, but that's a little ugly too. What do you think? 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. yea... I could probably do more surgery on Table/HdfsTable to reuse code from SchemaInfo, but was trying to stick to one of the high level goals of this project and minimize code churn on existing classes even if it means some copy-paste, except where such refactors are really trivial (extracting a straight-forward method). I initially attempted to refactor out the column-loading code from Table and reuse some of the same functions but it ended up being somewhat invasive there and didn't want to risk it. Happy to take another go at it, though, if you prefer 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? does num bytes get cached in the HMS? I think I missed that -- 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: Tue, 12 Jun 2018 23:35:22 +0000 Gerrit-HasComments: Yes
