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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Amos Bird <[email protected]> Gerrit-HasComments: Yes
