Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10713 )
Change subject: IMPALA-7140 (part 3): load partitions for FS tables ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@132 PS2, Line 132: Warehouse.makeSpecFromName > Good point. Makes sense to push down the column names in this api and agree that keeping these abstractions separate is preferable. http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@148 PS2, Line 148: name > Yea, it should. Do you think it's worth adding an assertion here that ret.k yes, would be an annoying thing to track down if differences leak in with how these names are constructed. http://gerrit.cloudera.org:8080/#/c/10713/2/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/10713/2/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@183 PS2, Line 183: List<String> names = Lists.newArrayList(); > ah, I'll add a Preconditions.checkState here that the partition specs are l yup, but its a public method, so good to be explicit. http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@279 PS2, Line 279: if (getNumClusteringCols() == 0) { > I can see the appeal but seems like almost all the rest of the code is the that's fine -- To view, visit http://gerrit.cloudera.org:8080/10713 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iddf2edbd6bdc0684560b2ecca9c4c6b6819ef1d3 Gerrit-Change-Number: 10713 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 15 Jun 2018 19:38:48 +0000 Gerrit-HasComments: Yes
