Alex Behm has posted comments on this change. Change subject: IMPALA-3724: Support Kudu non-covering range partitions ......................................................................
Patch Set 3: (14 comments) looks pretty good! http://gerrit.cloudera.org:8080/#/c/4856/3/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: Line 354: 2: optional bool lower_bound_type is_lower_bound_inclusive? Line 356: 4: optional bool upper_bound_type is_upper_bound_inclusive? http://gerrit.cloudera.org:8080/#/c/4856/3/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 1184: if (val.isEmpty()) { I don't think this case is possible Line 1194: if (val.isEmpty()) { I don't think this case is possible http://gerrit.cloudera.org:8080/#/c/4856/3/fe/src/main/java/org/apache/impala/analysis/DistributeParam.java File fe/src/main/java/org/apache/impala/analysis/DistributeParam.java: Line 139: // Impala does not check for overlapping range partitions; these checks are move to function comment http://gerrit.cloudera.org:8080/#/c/4856/3/fe/src/main/java/org/apache/impala/analysis/RangePartition.java File fe/src/main/java/org/apache/impala/analysis/RangePartition.java: Line 53: private final boolean lowerBoundType_; "Type" seems weird here, lowerBoundInclusive? Line 68: && lowerBoundValues.equals(upperBoundValues)); lowerBoundValues == upperBoundValues Line 110: analyzeBoundaryValues(upperBound_, analyzer); if (!isSingletonRange_) ? Line 121: "for range-partition bounds: %s", boundaryVal.toSql())); include the cause http://gerrit.cloudera.org:8080/#/c/4856/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: Line 1753: AnalysisError("create table tab (x int primary key) distribute by range (x) " + also try a constexpr that evaluates to NULL Line 1755: "for range-partition bounds: x + 1"); also add a negative test with some crazy expr like a subquery or analytic function? http://gerrit.cloudera.org:8080/#/c/4856/3/testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test File testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test: Line 338: # Insert row that are not covered by any of the existing range partitions are -> is Line 347: # Insert row that are not covered by any of the existing range partitions are -> is Line 348: INSERT IGNORE INTO kudu_test_tbl SELECT cast(id + 10000 as int), bool_col, tinyint_col, Let's take this up with MJ and see whether this is the behavior we want with IGNORE. Also think of the case where we are inserting millions of such record. Are we going to flood the coordinator with error messages? -- To view, visit http://gerrit.cloudera.org:8080/4856 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6799c01a37003f0f4c068d911a13e3f060110a06 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
