Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17937 )
Change subject: IMPALA-10970: Fix criterion for classifying coordinator only query ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/17937/3/be/src/scheduling/scheduler.h File be/src/scheduling/scheduler.h: http://gerrit.cloudera.org:8080/#/c/17937/3/be/src/scheduling/scheduler.h@77 PS3, Line 77: /// If there are only one TPlanExecInfo, single unpartitioned fragment is considered : /// coordinator only. : /// If there are multiple TPlanExecInfo, we should check all of them is co-located in : /// coordinator node. If all other TPlanExecInfo has only one fragments and it's : /// partition type is unpartitioned, we should also take this Query is a coordinator : /// only. (e.g. JOIN BUILD sink (MT_DOP>0) is co-located with its pre order ancestor : /// : JOIN_NODE) Check IMPALA-4224 for more detail. nit: I think the original comment is good enough, this is adding extra context for an edge case which is an implementation detail and does not need to me mentioned in the method comment http://gerrit.cloudera.org:8080/#/c/17937/3/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/17937/3/be/src/scheduling/scheduler.cc@824 PS3, Line 824: if (exec_request.plan_exec_info.size() > 1) { : /// There are intermediate join build side plan that co-located with join node, : /// don't take the query as coordinator only. : return false; : } we can instead do something like, num_parallel_plans = exec_request.plan_exec_info.size(); then at the end check for num_parallel_plans == 1, this way it makes the code self explanatory and helps us avoid adding comments. In addition to this, to further improve readability, you can add a better description to 'plan_exec_info' member in struct TQueryExecRequest in Query.thrift file as follows: 'The first plan in the list materializes the query result and subsequent plans materialize the build sides of joins. Each plan appears before its dependencies in the list.' http://gerrit.cloudera.org:8080/#/c/17937/3/testdata/workloads/functional-query/queries/QueryTest/mt-dop.test File testdata/workloads/functional-query/queries/QueryTest/mt-dop.test: http://gerrit.cloudera.org:8080/#/c/17937/3/testdata/workloads/functional-query/queries/QueryTest/mt-dop.test@30 PS3, Line 30: # TIMPALA-128: Coordinator only query determination error. : # From IMPALMA-4224 we execute join build in a separate fragments, the root : # plan contains two subplan, one of it contains join node while the other : # contains join builder as its root, the separate join build fragment is : # co-located with join node. : # This test case will fail due to outdated coordinator only determination : # logic which only take only the first subplan for concern. : # To ensure the inline left table placed at join probe side, we add : # STRAIGHT_JOIN hint to disable join inversion optimization. How about? IMPALA-10970: Verify that a query plan containing 2 parallel plans with the first one having a single unpartitioned fragment is correctly classified as not being a coordinator only query. This query create a plan with the aforementioned condition and has a join build fragment that will be co-located with the root sink fragment(IMPALMA-4224). This ensures that the query fails to schedule and eventually times out in the queue if incorrectly classified as a coordinator only query. To ensure the inline left table placed at join probe side, we add STRAIGHT_JOIN hint to disable join inversion optimization. -- To view, visit http://gerrit.cloudera.org:8080/17937 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icaaf1f1ba7a976122b4d37bd675e6d8181dc8700 Gerrit-Change-Number: 17937 Gerrit-PatchSet: 3 Gerrit-Owner: guojingfeng <guojingf...@tencent.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: guojingfeng <guojingf...@tencent.com> Gerrit-Comment-Date: Fri, 19 Nov 2021 00:43:04 +0000 Gerrit-HasComments: Yes
