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: (16 comments) http://gerrit.cloudera.org:8080/#/c/10713/2/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/10713/2/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java@171 PS2, Line 171: for (String partitionKey: hmsPartitionValues) this'll allow for a subset of partitioning columns to be returned for table. I assume that a subset would be incorrect so lets guard against it. It was not needed from the place this was factored since that list of strings came directly from the hms structure, so we can assume its correct. However, as a lib, who knows what those strings are. http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@546 PS2, Line 546: // only to end up throwing away the result. yikes... yes, just ran across this today as well. 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 something seems off here-- we're going back to hms for partitioning information. we should have this schema info already-- do we ever load partitions before the schema? if SchemaInfo were pulled out perhaps? I see that kinda breaks the layering here where objects (db, table) depend on metaprovider (and metaprovider knows nothing from above). other ideas? http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@143 PS2, Line 143: nit: consistent spacing http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@148 PS2, Line 148: name how does name correspond to the partition name that's passed in? I assume it must be a subset of the input names. http://gerrit.cloudera.org:8080/#/c/10713/2/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/10713/2/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java@122 PS2, Line 122: // TODO Auto-generated catch block remove http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java@144 PS2, Line 144: // TODO(todd): copy-paste from HdfsPartition mark it as expensive (like in the other place) 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@63 PS2, Line 63: /** nit: newline 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(); is loadPartitionSpecs() missing? http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@198 PS2, Line 198: nit: remove empty line http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@214 PS2, Line 214: if (spec.getName().isEmpty()) continue; when does this happen? 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) { perhaps this is modeled better as two classes, unpartitioned and partitioned. the place where LocalFsTable is created (LocalTable) gets the schema first, so has enough info to construct the right one. http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/local/LocalPartitionSpec.java File fe/src/main/java/org/apache/impala/catalog/local/LocalPartitionSpec.java: http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/local/LocalPartitionSpec.java@43 PS2, Line 43: private final ImmutableList<LiteralExpr> partitionValues; nit: _ suffix http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/local/LocalPartitionSpec.java@51 PS2, Line 51: getPartValuesFromPartName I see a similar method in CatalogOpExecutor (getPartValsFromName). Perhaps it can't be used, but one useful bit is that it guards against a bogus partitionValues being created for table (the partitioning columns must match). See the comment for parsePartitionKeyValues for a way to guard against this. http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java: http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java@60 PS2, Line 60: List<String> partitionNames) throws MetaException, TException; add a comment about what happens when not all partitions are loaded (could be caller's error or some other reason?). http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java File fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java: http://gerrit.cloudera.org:8080/#/c/10713/2/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java@111 PS2, Line 111: : /*checkFileDescriptors=*/false); nit: one line -- 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: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 15 Jun 2018 01:24:10 +0000 Gerrit-HasComments: Yes
