[Impala-ASF-CR] IMPALA-1654: Partition expr in DDL operations.

["Alex Behm (Code Review)" <ge...@cloudera.org>]

Alex Behm has posted comments on this change.

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


Patch Set 1:

(2 comments)

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 132:   // Transform <COL> = NULL into IsNull expr; <String COL> = '' into IsNull expr and
> Hmm, should I fix that together?
I'm ok with doing the fix separately if that's what you prefer. My thinking was that a lot of code here will go away for the fix.


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 1834:         } catch (NoSuchObjectException e) {
> Hive does this by making sure every partition expr should at least match on
Thanks for the info! After thinking this through with Greg, we've concluded this would be the best course of action with respect to IF EXISTS:

* The Hive behavior seems bizarre and somewhat arbitrary, so let's not adopt that.

* IF EXISTS seems generally not very useful, in particular, since we now return the number of dropped partitions. Ideally, we should deprecate IF EXISTS, but let's defer that until a later time and preserve compatibility as much as possible.

As a result, let's do the following:

* Existing cases with fully-specified partitions obey IF EXISTS just as before.

For example, consider a table partitioned by year and month.

ALTER TABLE DROP PARTITION (year=2009, month=1) should throw an error if the partition does not exist. It would not throw an error when IF EXISTS is specified.

The requirement is that all partition columns are mentioned with <SlotRef>=<Literal> just like in the old PartitionSpec.

This ensures that we won't break any existing scripts that may rely on the existing behavior of IF EXISTS.

* For the new cases where we drop partitions based on "arbirtary" exprs, we should just ignore IF EXISTS.

For example, ALTER TABLE DROP PARTITION (year = 2009) would always succeed, even if no partitions were dropped. The same applies to ALTER TABLE DROP PARTITION (month < 10) and so on.

Does this make sense to you? We maintain compatibility and the behavior for the new cases seems sane. Eventually, we should deprecate IF EXISTS imo.


-- 
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 <am...@gmail.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-HasComments: Yes