impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bikramjeet Vig (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5602: Fix query optimization for kudu and datasource tables
Date Wed, 23 Aug 2017 21:39:36 GMT
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

Mime
View raw message