impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1654: Partition expr in DDL operations.
Date Thu, 01 Sep 2016 04:57:31 GMT
Alex Behm has posted comments on this change.

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


Patch Set 3:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/3942/3/fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetLocationStmt.java
File fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetLocationStmt.java:

Line 86:             if (num == 3) break;
make 3 a static constant with a descriptive name


Line 91:               Joiner.on(", ").join(partitionNames), num<partitions.size() ? "..."
: "."));
style: space before and after "<"


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:     boolean ignore = false;
The old scenario requires that we have exactly one equality predicate for all partition columns,
and I don't think this logic captures that.

For example, if you have a table partitioned by a, b, c, then we want to obey IF EXISTS only
if there is exactly one <SlotRef>=<Literal> predicate for each partition column.

The current code will obey IF EXISTS in a cases like
ALTER TABLE DROP PARTITION (a=1, b=2) but it should not.

You may find Predicate.isSingleColumnPredicate() useful.

It would be nice to have this logic in a separate function with comment describing the scenarios
and the desire for backwards compatibility.

Also please add a test case like the one I mentioned above to make sure we do not obey IF
EXISTS then.


Line 99:         if (!(bp.getChild(0) instanceof SlotRef ||
The sides could be reversed. I'm not sure if we allowed that previously, but I think we should
treat those cases the same now.


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:         "Partition exprs cannot contain non-partition column int_col.");
we typically add a colon before the offending expr like this:

Partition exprs cannot contain non-partition column: int_col


Line 209:     // Drop partition using predicates
add comment that IF EXISTS is ignored here


Line 212:     AnalysisError("alter table functional.alltypes drop " +
should not be an error


Line 499:                   "year=2009/month=11 ... Partition expr in set location statements
" +
let's reverse the sentences in the error message, i.e. Partition expr in set ... should come
first


-- 
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: 3
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