impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Amos Bird (Code Review)" <ger...@cloudera.org>
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:

(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 <amosbird@gmail.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Amos Bird <amosbird@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message