[Impala-ASF-CR] IMPALA-10317: Add query option that limits huge joins at runtime

["Aman Sinha (Code Review)" <ge...@cloudera.org>]

Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/16706 )

Change subject: IMPALA-10317: Add query option that limits huge joins at runtime
......................................................................


Patch Set 3:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/16706/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/16706/3//COMMIT_MSG@11
PS3, Line 11: exceed
nit: 'exceeding' instead of exceed


http://gerrit.cloudera.org:8080/#/c/16706/3//COMMIT_MSG@18
PS3, Line 18: backend metrics for RowsReturned. Example from "select count(*) from
For this example, can you also add the query option ? I assume it was set to 10M ?


http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/runtime/coordinator.h@230
PS3, Line 230:     /// Total num rows produced by each join node
Pls indicate what the key of the map is. I am also wondering whether the string (which is the node name) is the only way to identify the node. Is there no numeric id that you can use ?


http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/runtime/coordinator.h@231
PS3, Line 231: per_num_join_rows_produced
This could more simply be named 'per_join_rows_produced'  (see similar comment elsewhere  on the option name)


http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:

http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/runtime/fragment-instance-state.cc@331
PS3, Line 331:         // stats join node
nit: this comment is a bit confusing.  Suggest rephrasing to 'row count stats for a join node'


http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/runtime/fragment-instance-state.cc@335
PS3, Line 335: rfind(nested_loop_type
A bit odd that for hash_type you are passing a second argument of 0 while not doing the same for nested_loop_type ? If there's a reason for that it would be good to add a comment.


http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/runtime/fragment-instance-state.cc@336
PS3, Line 336:           per_num_join_rows_produced[node->name()] = rows_counter->value();
Is there not a unique numeric id that can be used here instead of the node name ? (related comment in coordinator.h)


http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/service/impala-server.cc@2688
PS3, Line 2688:     Status err = Status::Expected(TErrorCode::JOIN_ROWS_PRODUCED_LIMIT_EXCEEDED,
it would be useful to know which join node (with the node id) in a complex plan exceeded this limit. Is it possible to print that info in the error message ?


http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/service/query-options.cc@991
PS3, Line 991:       case TImpalaQueryOptions::NUM_JOIN_ROWS_PRODUCED_LIMIT: {
I think the prefix 'NUM' can be removed and simplify the naming.  JOIN_ROWS_PRODUCED_LIMIT gives sufficient information that it is about the number of rows.  Similar simplification of the variable names can be done in other parts of the code.


http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/service/query-options.cc@996
PS3, Line 996:           return Status(Substitute("Invalid rows returned limit: '$0'. "
'rows returned limit' doesn't match the name of the query option. Should this be 'Invalid join rows produced limit'?


http://gerrit.cloudera.org:8080/#/c/16706/3/testdata/workloads/functional-query/queries/QueryTest/query-resource-limits.test
File testdata/workloads/functional-query/queries/QueryTest/query-resource-limits.test:

http://gerrit.cloudera.org:8080/#/c/16706/3/testdata/workloads/functional-query/queries/QueryTest/query-resource-limits.test@71
PS3, Line 71: ---- QUERY
Suggest adding a test with a mix of joins ..i.e couple of hash joins and a nested loop join where one or more of these might exceed the threshold.


http://gerrit.cloudera.org:8080/#/c/16706/3/testdata/workloads/functional-query/queries/QueryTest/query-resource-limits.test@87
PS3, Line 87: in 
nit: remove trailing whitespace



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idbca7e053b61b4e31b066edcfb3b0398fa859d02
Gerrit-Change-Number: 16706
Gerrit-PatchSet: 3
Gerrit-Owner: Fucun Chu <ch...@hotmail.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Dec 2020 08:11:26 +0000
Gerrit-HasComments: Yes