Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201708232139.v7NLdaa5030860@ip-10-146-233-104.ec2.internal>
Date: Wed, 23 Aug 2017 21:39:36 +0000
From: "Bikramjeet Vig (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Alex Behm <alex.behm@cloudera.com>, Matthew Jacobs <mj@cloudera.com>,
        Tim Armstrong <tarmstrong@cloudera.com>
Reply-To: bikramjeet.vig@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5602=3A_Fix_query_optimization_for_kudu_and_datasource_tables=0A?=
In-Reply-To: <gerrit.1501633638000.I93822d67ebda41d5d0456095c429e3915a3f40c4@gerrit.cloudera.org>
References: <gerrit.1501633638000.I93822d67ebda41d5d0456095c429e3915a3f40c4@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.7
archived-at: Wed, 23 Aug 2017 21:39:43 -0000

Bikramjeet Vig has posted comments on this change.

Change subject: IMPALA-5602: Fix query optimization for kudu and datasource tables
......................................................................


Patch Set 8:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/7560/8/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

PS8, Line 444: public void loadSchemaFromKudu() throws ImpalaRuntimeException
> Move this above loadSchema() so we have these functions clustered.
Done


http://gerrit.cloudera.org:8080/#/c/7560/8/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java
File fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java:

Line 513:   public boolean hasStorageLayerConjuncts() {
> single line? (and elsewhere)
Done


http://gerrit.cloudera.org:8080/#/c/7560/8/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
File fe/src/main/java/org/apache/impala/planner/KuduScanNode.java:

Line 526:   public boolean hasStorageLayerConjuncts() {
> single line?
Done


http://gerrit.cloudera.org:8080/#/c/7560/8/fe/src/main/java/org/apache/impala/planner/ScanNode.java
File fe/src/main/java/org/apache/impala/planner/ScanNode.java:

Line 217:    * Returns true if the node contains any conjuncts to be processed by Impala
> Returns true if this node has conjuncts to be evaluated by Impala against t
Done


Line 224:    * Returns true if the node contains any conjuncts pushed to the underlying storage
> Returns true if this node has conjuncts to be evaluated by the underlying s
Done


Line 228:     // Derived classes must override this method if they have any pushed conjuncts
> don't think we need this, then single line
Done


http://gerrit.cloudera.org:8080/#/c/7560/8/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:

Line 169:    * Add a new dummy HDFS or external Kudu table to the catalog based on the given CREATE
> Remove the "HDFS or external Kudu" part here. You already explain the limit
oops, forgot to remove this.


Line 199:     }else if (dummyTable instanceof KuduTable) {
> space after "}"
Done


PS8, Line 200:       if (!Table.isExternalTable(msTbl)) {
             :         fail("Failed to add table, external kudu table expected:\n" + createTableSql);
             :       }
> My concern with this approach is that this now does depend on Kudu, and I'm
This wont work for managed kudu tables because this method requires we only create a dummy table in the catalog and no actual table is created in kudu. This breaks planner code because when it creates a kudu scan node, the init method in the scan node connects to kudu client and checks if the table exists or not then throws an exception if it doesn't.


-- 
To view, visit http://gerrit.cloudera.org:8080/7560
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I93822d67ebda41d5d0456095c429e3915a3f40c4
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes