Amos Bird has posted comments on this change. Change subject: IMPALA-1654: Partition expr in DDL operations. ......................................................................
Patch Set 4: (38 comments) I'd like to take the following works. http://gerrit.cloudera.org:8080/#/c/3942/4/fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetLocationStmt.java File fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetLocationStmt.java: Line 42: public static final int NUM_PARTITION_LOG_LIMIT = 3; > private Done Line 86: num++; > style: ++num Done http://gerrit.cloudera.org:8080/#/c/3942/4/fe/src/main/java/com/cloudera/impala/analysis/ComputeStatsStmt.java File fe/src/main/java/com/cloudera/impala/analysis/ComputeStatsStmt.java: Line 360: for(HdfsPartition targetPartition: targetPartitions){ > style: space after "for" Done Line 530: partitionSet_ == null ? "" : partitionSet_.toSql(); > style: indent 4 from parent (like it was before) Done http://gerrit.cloudera.org:8080/#/c/3942/3/fe/src/main/java/com/cloudera/impala/analysis/PartitionSet.java File fe/src/main/java/com/cloudera/impala/analysis/PartitionSet.java: Line 94: > The old scenario requires that we have exactly one equality predicate for a Done Line 99: TupleDescriptor desc = analyzer.getDescriptor(tableName_.toString()); > The sides could be reversed. I'm not sure if we allowed that previously, bu Done http://gerrit.cloudera.org:8080/#/c/3942/4/fe/src/main/java/com/cloudera/impala/analysis/PartitionSet.java File fe/src/main/java/com/cloudera/impala/analysis/PartitionSet.java: Line 33: // Set during analysis > // Result of analysis Done Line 44: Preconditions.checkNotNull(tableName_); > please move the analysis code that is common between PartitionSet and Parti Done Line 46: analyzer.setEnablePrivChecks(false); > needs a brief comment like: Do not register column-authorization requests. Done Line 65: // SlotRef should be registered before getting slot ids > remove, should be clear why analysis is needed Done Line 80: // Make sure every conjunct only contains partition slot refs. > Would be cleaner to check this with conjunct.isBoundBySlotIds() like in the Done Line 117: // just as before, while more general partition expressions or partially-specified > just as before -> for backwards compatibility Done PS4, Line 118: > simply discard -> ignore Done Line 119: private void CheckIgnoreIfExists(Table table, List<Expr> transformedConjuncts) { > checkIgnoreIfExists() Done Line 121: Expr logExpr = null; > don't think we really need to log any offending expr Done Line 122: Set<String> columnNames = Sets.newHashSet(); > partColNames Done Line 125: if (Predicate.isEquivalencePredicate(e)) { > not quite correct: it must be an equality predicate Done Line 128: columnNames.add(slotRef.getRef().getDesc().getColumn().getName()); > add a Preconditions check to show that slotRef must belong to a partition c Done Line 136: columnNames.add(nullPredicate.getBoundSlot().getDesc().getColumn().getName()); > add same Preconditions check here Done Line 146: LOG.info(String.format( > Instead of this logging I think we should analyzer.addWarning(), but only i Done Line 149: } else if (columnNames.size() < table.getMetaStoreTable().getPartitionKeysSize()) { > use table.getNumClusteringCols() Done Line 150: // we don't need to check the non-partition columns here since it's already been > remove comment, should be clear after you've made the other changes above Done Line 169: if (Predicate.isEquivalencePredicate(e)) { > this will also transform <=> predicates, let's limit it to = predicates onl Done Line 202: String key = ToSqlUtils.getIdentSql(hdfsTable.getColumns() > Why do we need to use getIdentSql() here? Are you sure we need to quote ide I'm not sure. I was following the partitionSpec's way. http://gerrit.cloudera.org:8080/#/c/3942/4/fe/src/main/java/com/cloudera/impala/analysis/PartitionSpecBase.java File fe/src/main/java/com/cloudera/impala/analysis/PartitionSpecBase.java: Line 1: package com.cloudera.impala.analysis; > Apache copyright Done Line 9: * Created by amos on 8/18/16. > Replace this with a brief description of what this class is. Done Line 45: // Set the privilege requirement for this partition spec. Must be set prior to > newline before Done http://gerrit.cloudera.org:8080/#/c/3942/4/fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java: Line 893: * CatalogException if 'tbl' is not an HdfsTable. If the partitions in the given > update comment: null is never returned Done http://gerrit.cloudera.org:8080/#/c/3942/4/fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java File fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java: Line 1021: if (hdfsPartition != null) { > single line if it fits Done http://gerrit.cloudera.org:8080/#/c/3942/4/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: Line 1030: if (partitions == null) { > getPartitionsFromPartitionSet() will never return null so I think you want Done Line 1036: for(HdfsPartition partition: partitions) { > style: space after "for" Done Line 1748: * permanently deleted. > I think we should definitely move to the bulk API before we include this ch Cool, I would love to help. Should I leave my name on the TODO items? Line 1758: if (!ifExists) { > Let's change this logic and add Preconditions checks to reflect our new con I don't quite follow what you mean here. http://gerrit.cloudera.org:8080/#/c/3942/3/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java: Line 170: > we typically add a colon before the offending expr like this: Done Line 209: "No matching partition(s) found."); > add comment that IF EXISTS is ignored here Done Line 212: > should not be an error Done Line 499: "Partition exprs cannot contain non-partition column: int_col."); > let's reverse the sentences in the error message, i.e. Partition expr in se Done http://gerrit.cloudera.org:8080/#/c/3942/4/testdata/workloads/functional-query/queries/QueryTest/alter-table.test File testdata/workloads/functional-query/queries/QueryTest/alter-table.test: Line 251: 'Updated 1 table.' > remove the "1" since it may be misleading (we cannot update more than one t Done -- To view, visit http://gerrit.cloudera.org:8080/3942 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2c9162fcf9d227b8daf4c2e761d57bab4e26408f Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Amos Bird <amosb...@gmail.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Amos Bird <amosb...@gmail.com> Gerrit-HasComments: Yes
