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-4270: Gracefully fail unsupported queries with mt dop > 0.
Date Wed, 12 Oct 2016 18:37:25 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-4270: Gracefully fail unsupported queries with mt_dop > 0.
......................................................................


Patch Set 2:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/4677/2/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:

Line 272:           DCHECK_EQ(state->query_options().num_scanner_threads, 1);
> roll into a single dcheck with a compound predicate
Done


http://gerrit.cloudera.org:8080/#/c/4677/2/common/thrift/PlanNodes.thrift
File common/thrift/PlanNodes.thrift:

Line 205:   
> trailing
Done


Line 207:   // If this is true then the MT_DOP query option must be > 0.
> leave todo that it should be removed later
Done


http://gerrit.cloudera.org:8080/#/c/4677/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:

Line 314:    * Retrurns a set of file formats being scanned.
> spelling
Done


Line 562:     msg.hdfs_scan_node.setUse_mt_scan_nodeIsSet(useMtScanNode_);
> ..IsSet? it doesn't look like you're calling the actual setter
Weird. That seemed to work also. Fixed.


http://gerrit.cloudera.org:8080/#/c/4677/2/fe/src/main/java/org/apache/impala/planner/Planner.java
File fe/src/main/java/org/apache/impala/planner/Planner.java:

Line 206:     if (ctx_.getQueryOptions().mt_dop > 0) {
> the if is redundant, but you can make it a checkstate at the start of the f
Added Preconditions check.


http://gerrit.cloudera.org:8080/#/c/4677/2/fe/src/main/java/org/apache/impala/planner/PlannerContext.java
File fe/src/main/java/org/apache/impala/planner/PlannerContext.java:

Line 83:   public TQueryOptions getQueryOptions() { return getRootAnalyzer().getQueryOptions();
}
> why the change?
I added a convenience function Analyzer.getQueryOptions() and thought it would make sense
to use it here.


http://gerrit.cloudera.org:8080/#/c/4677/2/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java:

Line 155:    * - MT_DOP > 0 is not supported for plans with distributed joins or table
sinks.
> it's only supported for QueryStmts. take a look at Frontend.createExecReque
My thinking was that from a user's perspective running a statement with mt_dop > 0 should
have one of two outcomes:
1. The statement runs fine in multi-threaded mode.
2. The statement returns an error indicating that mt_dop is not supported for that statement.

If that's the behavior we want, then it's irrelevant whether we can actually generate parallel
plans for inserts/ctas.


http://gerrit.cloudera.org:8080/#/c/4677/2/testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
File testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test:

Line 51: |  tuple-ids=0,1 row-size=8B cardinality=unavailable
> what happened here?
functional_parquet has no stats.

I preferred to write the tests on Parquet because that's the  preferred file format, happy
to change it back or add more tests?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I91a60ea7b6e3ae4ee44be856615ddd3cd0af476d
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message