Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201611030511.uA35BDlV019738@ip-10-146-233-104.ec2.internal>
Date: Thu, 3 Nov 2016 05:11:13 +0000
From: "Dimitris Tsirogiannis (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mj@cloudera.com>, Alex Behm <alex.behm@cloudera.com>
Reply-To: dtsirogiannis@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3724=3A_Support_Kudu_non-covering_range_partitions=0A?=
In-Reply-To: <gerrit.1477520580000.I6799c01a37003f0f4c068d911a13e3f060110a06@gerrit.cloudera.org>
References: <gerrit.1477520580000.I6799c01a37003f0f4c068d911a13e3f060110a06@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Thu, 03 Nov 2016 05:11:20 -0000

Dimitris Tsirogiannis has posted comments on this change.

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


Patch Set 5:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4856/5/fe/src/main/java/org/apache/impala/analysis/DistributeParam.java
File fe/src/main/java/org/apache/impala/analysis/DistributeParam.java:

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.


http://gerrit.cloudera.org:8080/#/c/4856/5/fe/src/main/java/org/apache/impala/analysis/RangePartition.java
File fe/src/main/java/org/apache/impala/analysis/RangePartition.java:

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
Done


http://gerrit.cloudera.org:8080/#/c/4856/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java:

Line 1859:     AnalyzesOk("create table tab (a int primary key) distribute by range (a) " +
> // Test implicit casting/folding of partition values.
Done


http://gerrit.cloudera.org:8080/#/c/4856/5/testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
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 = false)
> add comment that this is testing implicit casting + folding
Done


-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes