impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Amos Bird (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-1654: Partition expr in DDL operations.
Date Thu, 18 Aug 2016 14:39:33 GMT
Amos Bird has posted comments on this change.

Change subject: IMPALA-1654: Partition expr in DDL operations.

Patch Set 1:

File be/src/service/

Line 937: void ImpalaServer::QueryExecState::SetResultSet() {
> make const TDdlExecResponse* a param to this function to make it clear when
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:

Line 1562: // A partition spec is a set of expressions used to select a list of partitions
> Update comment (still says 'partition spec')
File fe/src/main/java/com/cloudera/impala/analysis/

Line 79:               "can only match one partition.");
> in the error message include the names of the matched partitions
File fe/src/main/java/com/cloudera/impala/analysis/

Line 37:   PartitionSet getPartitionSet() {
> single line
File fe/src/main/java/com/cloudera/impala/analysis/

Line 442:             analysisResult_.isAlterTableStmt() ||
> Please take a look at my comment on the previous patch set. Summary: We sho
File fe/src/main/java/com/cloudera/impala/analysis/

Line 20:  * Represents a partition set - a collection of partition expressions.
> Represents a set of partitions resulting from evaluating a list of partitio

Line 31:   // does or does not contain any partition matched by the given partition expr.
> exprs

Line 34:   private Privilege privilegeRequirement_;
> There are a bunch of common members and analysis code between PartitionSpec

Line 44:   public String getTbl() {
> single lines where possible

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.

Line 103:     for (Expr e : conjuncts) {
> We still need to check that e only references partition columns - and no no

Line 105:       if (e.isLiteral()) {
> I think it's reasonable to disallow constant expressions here, so you can j

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.
File fe/src/main/java/com/cloudera/impala/catalog/

Line 390:   public List<HdfsPartition> getHdfsPartitions(Table tbl,
> do we really need this function? seems like the caller could do tbl.getPart
File fe/src/main/java/com/cloudera/impala/catalog/

Line 892:    * Drops the partition specified in 'partitionSpec' from 'tbl'. Throws a
> Update comment

Line 897:   public Table dropPartitions(Table tbl, List<List<TPartitionKeyValue>>
> partitionIds -> partitionSet
File fe/src/main/java/com/cloudera/impala/catalog/

Line 629:    * Gets hdfs partitions by partition id list.
> Update comment, remove @param

Line 983:    * Drops the partitions from HdfsTable. Cleans up its metadata from all the mappings
> Drops the given partitions from this table...

Line 985:    * Returns a list of partitions been dropped.
> Returns the list of partitions that were dropped.

Line 1923:    * @param partitionSet
> remove, we generally don't use the @param stuff

Line 1939:       // Get a list of HdfsPartition objects for the given partition ids.
> partition ids -> partition set
File fe/src/main/java/com/cloudera/impala/service/

Line 450:           alterTableSetTblProperties(tbl,
> please condense these lengthy function calls to wrap around like this:

Line 465:           alterTableUpdateStats(
> condense wrapping

Line 486:             alterPartitionSetCached(tbl,
> wrapping

Line 503:         loadTableMetadata(tbl,
> wrapping

Line 1784:    * Drops existing partitions from the given table in Hive. If the partition is
> If a partition is cached ...

Line 1810:     List<HdfsPartition> parts = catalog_.getHdfsPartitions(tbl, partitionSet);
> why not use tbl.getPartitionsFromPartitionSet() directly and get rid of Cat

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?

Line 1978:       numUpdatedPartitions.setRef(numTargetPartitions);
> can't you set to modifiedParts.size() directly?

Line 2057:       numUpdatedPartitions.setRef(numTargetPartitions);
> use modifiedParts.size() directly?

Line 2246:         ++ numTargetPartitions;
> extra space

Line 2616:   private void bulkDropPartitions(String dbName, String tableName,
> please remove this work-in-progress from the patch set, I'm still thinking 
File shell/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I2c9162fcf9d227b8daf4c2e761d57bab4e26408f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Amos Bird <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Amos Bird <>
Gerrit-HasComments: Yes

View raw message