impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3724: Support Kudu non-covering range partitions
Date Thu, 03 Nov 2016 05:11:13 GMT
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-3724: Support Kudu non-covering range partitions

Patch Set 5:

File fe/src/main/java/org/apache/impala/analysis/

Line 137:     for (String colName: colNames_) pkColDefs.add(pkColumnDefByName_.get(colName));
> is pkColumnDefByName_.get(colName) guaranteed to be non-NULL?
Yes, we've already checked that in analyze() before the call to this function.
File fe/src/main/java/org/apache/impala/analysis/

Line 150:     LiteralExpr literal = LiteralExpr.create(value, analyzer.getQueryCtx());
> Can't we move this after the implicit casting? That way we'll only have one
I have one example where this doesn't seem to work. e.g. PARTITION VALUES < factorial(4),
where the partition column is type INT. Because factorial returns type BIGINT the check in
L162 throws an error. However, by doing the folding first the checks pass as the return value
can be implicitly casted to INT without precision loss. Thoughts?

Line 169:       Expr castedLiteral = literal.uncheckedCastTo(colType);
> castLiteral
File fe/src/test/java/org/apache/impala/analysis/

Line 1859:     AnalyzesOk("create table tab (a int primary key) distribute by range (a) "
> // Test implicit casting/folding of partition values.
File testdata/workloads/functional-query/queries/QueryTest/kudu_create.test:

Line 82: create table tab (a int primary key) distributed by range (a) (partition value =
> add comment that this is testing implicit casting + folding

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I6799c01a37003f0f4c068d911a13e3f060110a06
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-HasComments: Yes

View raw message