Fucun Chu 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 4:

(12 comments)

Thanks for the review! Addressed the 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
Done


http://gerrit.cloudera.org:8080/#/c/16706/3//COMMIT_MSG@18
PS3, Line 18: backend metrics for RowsReturned. Example from "
> For this example, can you also add the query option ? I assume it was set t
Done


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. The key is 
join node id.
> Pls indicate what the key of the map is. I am also wondering whether the st
The key has been changed to node id


http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/runtime/coordinator.h@231
PS3, Line 231: join_rows_produced;
> This could more simply be named 'per_join_rows_produced'  (see similar comm
Done


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:         // row count stats for a join node
> nit: this comment is a bit confusing.  Suggest rephrasing to 'row count sta
Done


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 n
To implement the startsWith function, the two variable judgments are the same.
Done


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


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
Done


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::USE_DOP_FOR_COSTING: {
> I think the prefix 'NUM' can be removed and simplify the naming.  JOIN_ROWS
Done


http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/service/query-options.cc@996
PS3, Line 996:         StringParser::ParseResult result;
> 'rows returned limit' doesn't match the name of the query option. Should th
Done


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
Done


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
Done



--
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: 4
Gerrit-Owner: Fucun Chu <chufu...@hotmail.com>
Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com>
Gerrit-Reviewer: Fucun Chu <chufu...@hotmail.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Comment-Date: Sun, 20 Dec 2020 04:12:02 +0000
Gerrit-HasComments: Yes

Reply via email to