Daniel Vanko has posted comments on this change. ( http://gerrit.cloudera.org:8080/22496 )
Change subject: IMPALA-13074: Add sink node to Web UI's graphical plan for DDL/DML queries ...................................................................... Patch Set 10: (11 comments) Thank you for all your suggestions. IMO there's two ways to add testing. 1. If we want tests for this change as well, first we should create the base tests in IMPALA-13824 and then extend them in this change. 2. After the submission of this change as-is, we will go forward with IMPALA-13824, testing the modifications included in this change as well. What do you think? http://gerrit.cloudera.org:8080/#/c/22496/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22496/8//COMMIT_MSG@7 PS8, Line 7: Add sink node to Web UI's graphical plan for DDL/DML q > Please use something like... Done http://gerrit.cloudera.org:8080/#/c/22496/8//COMMIT_MSG@14 PS8, Line 14: * tested manually on WebUI with CTAS statements : * with UPDATE statements on Ic > Breakdown to more points if necessary Done http://gerrit.cloudera.org:8080/#/c/22496/8/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/22496/8/be/src/service/impala-http-handler.cc@1069 PS8, Line 1069: Summary is stored with -1 as id if it is for a data sink at the root of a fragment > nit: Summary [...] fragment. Done http://gerrit.cloudera.org:8080/#/c/22496/8/be/src/service/impala-http-handler.cc@1146 PS8, Line 1146: Value > nit: no need for std::, as "common/names.h" is included Done http://gerrit.cloudera.org:8080/#/c/22496/8/be/src/service/impala-http-handler.cc@1153 PS8, Line 1153: label_detail_str = sink.child_data_sink > no need of this enclosing if stmt, as the for-loop starts from i = 1 Done http://gerrit.cloudera.org:8080/#/c/22496/8/be/src/service/impala-http-handler.cc@1160 PS8, Line 1160: label_detail_str = to_string(sink.table_sink.action); : break; > Without default case, compilation errors are present: Added DCHECK. http://gerrit.cloudera.org:8080/#/c/22496/8/be/src/service/impala-http-handler.cc@1168 PS8, Line 1168: > If the summary must be always there, we could have a DCHECK instead/also. Done http://gerrit.cloudera.org:8080/#/c/22496/8/be/src/service/impala-http-handler.cc@1181 PS8, Line 1181: } : : // Helper method which converts a list of plan fragments into a single JSON document, with : // the following schema: : // "plan_nodes": [ : // { : // "label": "12:AGGREGATE", : // "label_detail": "FINALIZE", : // "output_card": 23456, : // "num_instances": 34, : // "max_time": "1m23s", : // "avg_time": "1.3ms", : // "children": [ : // { : // "label": "11:EXCHANGE", : // "label_detail": "UNPARTITIONED", : // "children": [] : // } : // ] : // }, : // { : // "label": "07:AGGREGATE", : // "label_detail": "", : // "children": [], : // > Please update the example here, unless redundant Schema does not change, so I think it's not necessary. http://gerrit.cloudera.org:8080/#/c/22496/8/be/src/service/impala-http-handler.cc@1219 PS8, Line 1219: PlanNodeId, TPlanNodeExecSummary> exec_summaries; : for (const TPlanNodeExecSummary& s: summary.nodes) { : // All sink has -1 as node_id, w > nit: All [...] present. Done http://gerrit.cloudera.org:8080/#/c/22496/8/be/src/service/impala-http-handler.cc@1231 PS8, Line 1231: ragment.__isset.output_sink && > To support MERGE statements as well, you also need to add MERGE_SINK. Done http://gerrit.cloudera.org:8080/#/c/22496/8/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/22496/8/common/thrift/Query.thrift@1043 PS8, Line 1043: E > nit: capital E Done -- To view, visit http://gerrit.cloudera.org:8080/22496 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2bd442f6499efde7406d87c2b1fd1b46a45381b Gerrit-Change-Number: 22496 Gerrit-PatchSet: 10 Gerrit-Owner: Daniel Vanko <[email protected]> Gerrit-Reviewer: Daniel Vanko <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 13 Mar 2025 12:33:35 +0000 Gerrit-HasComments: Yes
