impala-reviews 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-3724: Support Kudu non-covering range partitions
Date Fri, 28 Oct 2016 01:25:05 GMT
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 <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message