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.b...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-HasComments: Yes
