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: (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 > why not? I agree with not slavish adherence to Javadoc when the parameters It was my understanding that flowing the param name in the description is preferable to using @param. I see that the project is inconsistent... some places use it, some don't, even in the same file. The several places where I've looked, it introduces duplication (e.g., Expr.castChild). Since we're far from being consistent, the @param doesn't seem to buy us much, so its terser to omit it. Open to alternatives though. 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; > the problem is I needed to use tblName on line 75, so I need to checkNotNul I think this looks fine: this.db_ = Preconditions.checkNotNull(db); this.name_ = Preconditions.checkNotNull(tblName); this.schemaInfo = Preconditions.checkNotNull(schemaInfo); Preconditions.checkArgument(tblName.toLowerCase().equals(tblName)); 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 thats fine, but pls leave a breadcrumb for where some of the lifting comes from. http://gerrit.cloudera.org:8080/#/c/10630/4/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@191 PS4, Line 191: stats > does num bytes get cached in the HMS? I think I missed that yes, have a look at setTableStats in Table.java -- 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:09:31 +0000 Gerrit-HasComments: Yes
