Alex Behm has posted comments on this change. Change subject: IMPALA-3724: Support Kudu non-covering range partitions ......................................................................
Patch Set 2: (11 comments) First pass, need to look closer at the tests. http://gerrit.cloudera.org:8080/#/c/4856/2/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: Line 355: 2: optional ExternalDataSource.TComparisonOp lower_bound_op Seems weird to reference ExternalDataSource here. Wouldn't a bool be sufficient to indicate inclusive/exclusive? http://gerrit.cloudera.org:8080/#/c/4856/2/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 1192: literal:l LESSTHAN We need an Expr and constant folding here to support negative ranges like "PARTITION -10 <= VALUES <= 10". Yes, our parsing of that is quite unfortunate. http://gerrit.cloudera.org:8080/#/c/4856/2/fe/src/main/java/org/apache/impala/analysis/DistributeParam.java File fe/src/main/java/org/apache/impala/analysis/DistributeParam.java: Line 35: * clause of a CREATE TABLE statement. The distribution can be hash-based or add something like "See RangePartition for details on the supported range partitions." Line 36: * range-based. or both. Line 91: // Primary key columns of this distribution. If no columns are specified, all Columns of this distribution? Seem better to not overload 'primary key' with a different meaning. Line 92: // the primary key columns of the associated table as used. as -> are Line 139: if (!lowerBound.isEmpty() && lowerBound.size() != colNames_.size()) { Are we deferring checks like conflicting unbounded ranges to Kudu? Either add such checks or add comment that we defer to Kudu. Examples: RANGE (col) PARTITION 10 <= VALUE, PARTITION 20 <= VALUE or RANGE (col) PARTITION VALUE <= 20, PARTITION VALUE <= 50 Same thing for overlapping ranges. Line 141: "partition values is different than the number of projected key " + projected key columns -> distribution columns? Line 174: "(type: %s) is not type compatible with column '%s' (type: %s).", with distribution column Line 191: builder.append(" ("); missing "RANGE(col)"? http://gerrit.cloudera.org:8080/#/c/4856/2/fe/src/main/java/org/apache/impala/analysis/RangePartition.java File fe/src/main/java/org/apache/impala/analysis/RangePartition.java: Line 56: private RangePartition(List<LiteralExpr> lowerBoundValues, BinaryPredicate.Operator lOp, Why not represent the OP as a bool inclusive/exclusive? Seems simpler with the thrift and also eliminates the possibility of nonsensical OPs (like <=>). -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-HasComments: Yes
