Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19214 )
Change subject: IMPALA-7969: Always admit trivial queries immediately ...................................................................... Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/19214/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19214/1//COMMIT_MSG@10 PS1, Line 10: immediate admission > Do you know if these trivial queries consume some global resource we can ru Added a limit of 3 for the concurrent running trivial queries per resource pool. I think the target in this patch is to allow immediate admission without causing too many troubles, so adding restrictions should be okay. http://gerrit.cloudera.org:8080/#/c/19214/1//COMMIT_MSG@14 PS1, Line 14: UNION > This could be extended with the union being all const. Added a restriction that only zero or one row can be returned. http://gerrit.cloudera.org:8080/#/c/19214/1//COMMIT_MSG@18 PS1, Line 18: - select * from table limit 0; > is this also a valid example? Added a restriction that the max number of rows is 1. So "select 1 union all select 2" won't be trivial in this patch. http://gerrit.cloudera.org:8080/#/c/19214/1//COMMIT_MSG@21 PS1, Line 21: Need some thinking on the definition of tirvial query : - Any other cases can be trivial queries? : - Any case that we don't want but allows for immediate admission in : this patch? > I think probably okay to create a separate query option for admitting coord I implement the trivial query as a subset of coord-only query in this patch, because would like to keep it simple in the first version, might extend later. http://gerrit.cloudera.org:8080/#/c/19214/1/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/19214/1/be/src/scheduling/admission-controller.cc@1889 PS1, Line 1889: if (state->GetIsTrivialQuery()) { : VLOG_QUERY << "Admiited by trivial query policy."; : queue_node->admitted_schedule = std::move(group_state.state); : return true; : } > This will always add trivial queries to the first executor group. Can this I think in the first version we can treat the trivial query as a subset of coord-only query to keep it simple. Added a DCHECK to ensure it is a coord-only query. http://gerrit.cloudera.org:8080/#/c/19214/1/fe/src/main/java/org/apache/impala/planner/TrivialQueryChecker.java File fe/src/main/java/org/apache/impala/planner/TrivialQueryChecker.java: http://gerrit.cloudera.org:8080/#/c/19214/1/fe/src/main/java/org/apache/impala/planner/TrivialQueryChecker.java@44 PS1, Line 44: String displayLabel = planRoot.getDisplayLabel(); : if (displayLabel == null) return false; : if (!displayLabel.equals("00:EMPTYSET") && !displayLabel.equals("00:UNION")) { : return false; : } > Can the root node have child nodes? My guess is no, as empty set has no chi Added a restriction that trivial query only allow to return 0 or 1 row. http://gerrit.cloudera.org:8080/#/c/19214/1/fe/src/main/java/org/apache/impala/planner/TrivialQueryChecker.java@60 PS1, Line 60: sinkRoot.collectExprs(exprs); > Will this detect a sleep in the source nodes e.g. select 1 union all select Make a restriction that no more than 1 row can be returned, so "select 1 union all select sleep(1000)" won't be accepted as a trivial query in this patch. Also added it as a negative case in the ee test. http://gerrit.cloudera.org:8080/#/c/19214/1/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/19214/1/tests/custom_cluster/test_admission_controller.py@1202 PS1, Line 1202: "Admission result: Admitted immediately > It would be nice to able to check somehow whether a query was handled as tr Added a string AdmissionController::PROFILE_INFO_VAL_ADMIT_TRIVIAL: "Admitted as a trivial query". http://gerrit.cloudera.org:8080/#/c/19214/1/tests/custom_cluster/test_admission_controller.py@1208 PS1, Line 1208: select 1,3 > Can you add a test for a more complex constant query, e.g select 1 union al Added a testcase "select 1 union all select 2" as a negative case. Because restrict trivial query to have no more than 1 row. -- To view, visit http://gerrit.cloudera.org:8080/19214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a729764e3055d7eb11900c96c82ff53eb261f91 Gerrit-Change-Number: 19214 Gerrit-PatchSet: 1 Gerrit-Owner: Yida Wu <wydbaggio...@gmail.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Yida Wu <wydbaggio...@gmail.com> Gerrit-Comment-Date: Thu, 22 Dec 2022 18:12:13 +0000 Gerrit-HasComments: Yes
