Amos Bird has posted comments on this change. Change subject: IMPALA-1654: Partition expr in DDL operations. ......................................................................
Patch Set 1: (36 comments) http://gerrit.cloudera.org:8080/#/c/3942/1/be/src/service/query-exec-state.cc File be/src/service/query-exec-state.cc: Line 937: void ImpalaServer::QueryExecState::SetResultSet() { > make const TDdlExecResponse* a param to this function to make it clear when Done http://gerrit.cloudera.org:8080/#/c/3942/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 897: // Need to check partition is an opt_partition_spec in analyzer to avoid > suggest minor rephrase: Done Line 1562: // A partition spec is a set of expressions used to select a list of partitions > Update comment (still says 'partition spec') Done http://gerrit.cloudera.org:8080/#/c/3942/1/fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetLocationStmt.java File fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetLocationStmt.java: Line 79: "can only match one partition."); > in the error message include the names of the matched partitions Done http://gerrit.cloudera.org:8080/#/c/3942/1/fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetStmt.java File fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetStmt.java: Line 37: PartitionSet getPartitionSet() { > single line Done http://gerrit.cloudera.org:8080/#/c/3942/1/fe/src/main/java/com/cloudera/impala/analysis/AnalysisContext.java File fe/src/main/java/com/cloudera/impala/analysis/AnalysisContext.java: Line 442: analysisResult_.isAlterTableStmt() || > Please take a look at my comment on the previous patch set. Summary: We sho Done http://gerrit.cloudera.org:8080/#/c/3942/1/fe/src/main/java/com/cloudera/impala/analysis/PartitionSet.java File fe/src/main/java/com/cloudera/impala/analysis/PartitionSet.java: Line 20: * Represents a partition set - a collection of partition expressions. > Represents a set of partitions resulting from evaluating a list of partitio Done Line 31: // does or does not contain any partition matched by the given partition expr. > exprs Done Line 34: private Privilege privilegeRequirement_; > There are a bunch of common members and analysis code between PartitionSpec Done Line 44: public String getTbl() { > single lines where possible Done Line 93: "Dynamic partition spec is not supported."); > Error is somewhat cryptic. It should contain: After a second thought, I think we may allow a single slotref in partition expr. This looks like a dynamic partition spec but is indeed an expr and may be useful if we have bool columns. Moreover, it will fail if the slotref not returns bool. Line 99: // Verify the partition exprs. Make sure every conjunct only contains exactly > Update comment. Done Line 103: for (Expr e : conjuncts) { > We still need to check that e only references partition columns - and no no Done Line 105: if (e.isLiteral()) { > I think it's reasonable to disallow constant expressions here, so you can j Done Line 132: // Transform <COL> = NULL into IsNull expr; <String COL> = '' into IsNull expr and > The NULL transformation looks good, but I don't think the string transforma The old kv style partition spec is case insensitive, and it treats empty string as null partition. They are needed for backward compatibitily. http://gerrit.cloudera.org:8080/#/c/3942/1/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java: Line 390: public List<HdfsPartition> getHdfsPartitions(Table tbl, > do we really need this function? seems like the caller could do tbl.getPart Done http://gerrit.cloudera.org:8080/#/c/3942/1/fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java: Line 892: * Drops the partition specified in 'partitionSpec' from 'tbl'. Throws a > Update comment Done Line 897: public Table dropPartitions(Table tbl, List<List<TPartitionKeyValue>> partitionIds) > partitionIds -> partitionSet Done http://gerrit.cloudera.org:8080/#/c/3942/1/fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java File fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java: Line 629: * Gets hdfs partitions by partition id list. > Update comment, remove @param Done Line 983: * Drops the partitions from HdfsTable. Cleans up its metadata from all the mappings > Drops the given partitions from this table... Done Line 985: * Returns a list of partitions been dropped. > Returns the list of partitions that were dropped. Done Line 1923: * @param partitionSet > remove, we generally don't use the @param stuff Done Line 1939: // Get a list of HdfsPartition objects for the given partition ids. > partition ids -> partition set Done http://gerrit.cloudera.org:8080/#/c/3942/1/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: Line 450: alterTableSetTblProperties(tbl, > please condense these lengthy function calls to wrap around like this: Done Line 465: alterTableUpdateStats( > condense wrapping Done Line 486: alterPartitionSetCached(tbl, > wrapping Done Line 503: loadTableMetadata(tbl, > wrapping Done Line 1784: * Drops existing partitions from the given table in Hive. If the partition is cached, > If a partition is cached ... Done Line 1810: List<HdfsPartition> parts = catalog_.getHdfsPartitions(tbl, partitionSet); > why not use tbl.getPartitionsFromPartitionSet() directly and get rid of Cat Done Line 1834: } catch (NoSuchObjectException e) { > The error behavior is a somewhat strange now because a user can write ALTER Hive does this: ``` if ((parts.size() < minCount) && !ifExists) { throw new NoSuchObjectException("Some partitions to drop are missing"); ``` I think we should have a transactional behavior when dropping multiple partitions. Then we can rollback if not ifExists. Line 1969: long numTargetPartitions = 0L; > unused? Done Line 1978: numUpdatedPartitions.setRef(numTargetPartitions); > can't you set to modifiedParts.size() directly? Done Line 2057: numUpdatedPartitions.setRef(numTargetPartitions); > use modifiedParts.size() directly? Done Line 2246: ++ numTargetPartitions; > extra space Done Line 2616: private void bulkDropPartitions(String dbName, String tableName, > please remove this work-in-progress from the patch set, I'm still thinking Done http://gerrit.cloudera.org:8080/#/c/3942/1/shell/impala_client.py File shell/impala_client.py: Line 450: excluded_query_types = ['use', 'drop'] > Did you do the manual testing I suggested in the last round? I did. Those not returning a result set just return 0 row. I suppose it works fine. -- 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: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Amos Bird <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Amos Bird <[email protected]> Gerrit-HasComments: Yes
