Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21257 )
Change subject: IMPALA-12980: Translate CpuAsk into admission control slots ...................................................................... Patch Set 11: (6 comments) http://gerrit.cloudera.org:8080/#/c/21257/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21257/11//COMMIT_MSG@19 PS11, Line 19: rather : than sum of it (48) Can this get higher than the amount of slots per executor? Based on IMPALA-8998 the query is refused in this case. http://gerrit.cloudera.org:8080/#/c/21257/11//COMMIT_MSG@28 PS11, Line 28: which will be a closer resemblance of maximum : parallel execution of fragment instances. Does PLANNER_CPU_ASK always calculate a greater or equal number of slots than LARGEST_FRAGMENT? http://gerrit.cloudera.org:8080/#/c/21257/11/testdata/workloads/functional-query/queries/QueryTest/processing_cost_admission_slots.test File testdata/workloads/functional-query/queries/QueryTest/processing_cost_admission_slots.test: http://gerrit.cloudera.org:8080/#/c/21257/11/testdata/workloads/functional-query/queries/QueryTest/processing_cost_admission_slots.test@2 PS11, Line 2: ---- QUERY It would be very nice to add an explain for the same query with the same query options to show plan. At the moment it is hard for me to think through how exactly we got to the results below - seeing a plan + having a few pointers in comments could help a lot. Besides documentation, this would also ensure that if the plan changes in the future, the test case with the explain will break, making it much easier to investigate the cause of the failure. http://gerrit.cloudera.org:8080/#/c/21257/11/testdata/workloads/functional-query/queries/QueryTest/processing_cost_admission_slots.test@31 PS11, Line 31: ---- RESULTS Are the results actually important here? If the goal is to verify that slot_count_strategy doesn't change the results of the queries, then it would be better to add it to the dimensions of some large query suite, e.g. test_queries.py http://gerrit.cloudera.org:8080/#/c/21257/11/tests/custom_cluster/test_executor_groups.py File tests/custom_cluster/test_executor_groups.py: http://gerrit.cloudera.org:8080/#/c/21257/11/tests/custom_cluster/test_executor_groups.py@1245 PS11, Line 1245: # CoreCount={total=16 trace=F15:3+F01:1+F14:3+F03:1+F13:3+F05:1+F12:3+F07:1}, This is very useful to help in understanding the patch, but it would be better to point to specific query plan, as the plan (and fragment numbers) may change in future. http://gerrit.cloudera.org:8080/#/c/21257/11/tests/query_test/test_processing_cost.py File tests/query_test/test_processing_cost.py: http://gerrit.cloudera.org:8080/#/c/21257/11/tests/query_test/test_processing_cost.py@42 PS11, Line 42: add_mandatory_exec_option(cls, 'slot_count_strategy', 'planner_cpu_ask') IMO moving these to SET statements in the .test file would be clearer. It would also allow running the same query with both slot_count_strategy and planner_cpu_ask to show that it leads to different estimates. -- To view, visit http://gerrit.cloudera.org:8080/21257 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I338ca96555bfe8d07afce0320b3688a0861663f2 Gerrit-Change-Number: 21257 Gerrit-PatchSet: 11 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.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: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Thu, 11 Apr 2024 14:21:19 +0000 Gerrit-HasComments: Yes
