Amos Bird has posted comments on this change. Change subject: IMPALA-1654: General partition exprs in DDL operations. ......................................................................
Patch Set 6: (25 comments) http://gerrit.cloudera.org:8080/#/c/3942/6//COMMIT_MSG Commit Message: PS6, Line 7: > General partition exprs in DDL opreations. Done Line 10: now use compound predicates to specify a list of partitions in statement > statements Done Line 56: The only senario that allows partition predicates to return empty > Update since we've slightly changed the behavior. Done http://gerrit.cloudera.org:8080/#/c/3942/6/fe/src/main/java/com/cloudera/impala/analysis/PartitionSet.java File fe/src/main/java/com/cloudera/impala/analysis/PartitionSet.java: Line 61: Preconditions.checkState(table_ instanceof HdfsTable); > L61 - L64 can also go in super.analyze(). Done http://gerrit.cloudera.org:8080/#/c/3942/6/fe/src/main/java/com/cloudera/impala/analysis/PartitionSpec.java File fe/src/main/java/com/cloudera/impala/analysis/PartitionSpec.java: Line 121: // Only HDFS tables are partitioned. > L121 - L124 can also go in super.analyze() Done http://gerrit.cloudera.org:8080/#/c/3942/6/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: Line 1031: if (partitions.isEmpty()) { return; } > remove the { } for single-line ifs Done http://gerrit.cloudera.org:8080/#/c/3942/6/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java: Line 106: // Data types don't match > Remove this comment and move the test right under the previous one. In gene Done Line 127: // Data types don't match > remove, already tested above Done Line 157: // NULL partition keys > Can this go in the loop above? Should be the same for add and drop. Done Line 165: // Data types don't match > remove, already tested above Done Line 185: AnalysisError("alter table functional.alltypes drop " + > Do these really return an error? Seems like they shouldn't. Done Line 204: // IF EXISTS properly checks for partition existence > with a fully specified partition. Done Line 215: AnalyzesOk("alter table functional.alltypes drop " + > Add a negative test case with a constant partition expr. Done Line 217: AnalyzesOk("alter table functional.alltypes drop " + > also add a variant of this test that has PARTITION(year>9050 and month=10) Done Line 219: AnalyzesOk("alter table functional.alltypes drop " + > also add a test that shows something like this is ok: Done Line 221: AnalyzesOk("alter table functional.alltypes drop if exists " + > Add the expected warning as a param to AnalyzesOK() The default behavior is to add IF EXISTS to all the general partition expr ddl in order to relax the constrain. Line 224: // Not a valid column > remove, already tested above Done http://gerrit.cloudera.org:8080/#/c/3942/6/testdata/workloads/functional-query/queries/QueryTest/partition-ddl-predicates.test File testdata/workloads/functional-query/queries/QueryTest/partition-ddl-predicates.test: Line 3: use ddl_test_db > remove, we're using the unique_database Done Line 7: # First create an partitioned table > a partitioned table Done Line 11: ---- QUERY > no need for this, should be clear that there are no partitions Done Line 20: alter table p1 add partition (j=1,k="a"); > you can move these directly under the create table after L8 Done Line 22: alter table p1 add partition (j=1,k="c"); > add partitions with NULLs and some tests Done Line 42: show files in p1 partition (j<2, k="a") > let's insert some data so there is something to show here Done Line 144: alter table p1 drop partition (j=2, k="bla") > remove, already covered by analyzer test Done Line 151: '2','d',-1,0,regex:.+,regex:.+,regex:.+,regex:.+,regex:.+,regex:.*/test-warehouse/ddl_test_db.db/p1/j=2/k=d > leave the format column since you've altered the format of some partitions Done -- 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: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Amos Bird <amosb...@gmail.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Amos Bird <amosb...@gmail.com> Gerrit-HasComments: Yes
