Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10735 )
Change subject: IMPALA-7140 (part 4): support creating descriptors for FS tables ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/10735/3/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/10735/3/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java@295 PS3, Line 295: THdfsPartition > naming does not align with the switch from 'hdfs' to 'fs'. pls add a todo t added a TODO on the Thrift file http://gerrit.cloudera.org:8080/#/c/10735/3/fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java File fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java: http://gerrit.cloudera.org:8080/#/c/10735/3/fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java@160 PS3, Line 160: getFilteredHmsParameters > not for this change, but wondering if hms details need to be exposed this h I separated this API rather than filtering at the call site because I'd like to be able to push down the filtering all the way to what we fetch from HMS after it implements something like https://issues.apache.org/jira/browse/HIVE-19715 . We might want to _always_ remove the incremental stats from the parameters and add an entirely separate getter which returns them in decoded format for memory usage - I think Parna is working on something like that. But didn't want to expand this change. I'll add a TODO about this. http://gerrit.cloudera.org:8080/#/c/10735/3/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java File fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java: http://gerrit.cloudera.org:8080/#/c/10735/3/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java@87 PS3, Line 87: 0; > interesting that this one can be 0, L73 returns empty but flipping L81 to t yea, I was surprised too. It's temporary at least until the next patch in this series. http://gerrit.cloudera.org:8080/#/c/10735/3/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java@98 PS3, Line 98: -1 > what's the -1? Done http://gerrit.cloudera.org:8080/#/c/10735/3/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java: http://gerrit.cloudera.org:8080/#/c/10735/3/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@175 PS3, Line 175: > add a comment that a slim hdfs table is being constructed (no hdfs info, no I inlined this call into the above function since it was the only call site, and the above function name makes it clearer it's just a descriptor. The "no HDFS info" bit is just temporary until the next patch, not something inherent about the design here (it will include the descriptors) http://gerrit.cloudera.org:8080/#/c/10735/3/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@176 PS3, Line 176: get > nit: build instead of get? obviated by above -- To view, visit http://gerrit.cloudera.org:8080/10735 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4550612eb6d1e3a324f49a9c4d24b048e45d3738 Gerrit-Change-Number: 10735 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 22 Jun 2018 16:59:12 +0000 Gerrit-HasComments: Yes
