Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/22799 )
Change subject: IMPALA-13824: add unit tests for PlanToJson ...................................................................... Patch Set 4: (15 comments) Thanks for working on this, I think the general approach is great. Found a few nitpicks though. http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc File be/src/service/impala-http-handler-test.cc: http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@28 PS4, Line 28: using namespace impala; It's not neeed since you've put everything to the impala namespace http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@35 PS4, Line 35: nodeId In C++, for variable names we don't use camelCase, we use underscores, i.e. it should be 'node_id' http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@57 PS4, Line 57: nit: parameter lines need +4 spaces, not +2 http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@58 PS4, Line 58: string& nit: could be const? http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@61 PS4, Line 61: ) { nit: this could be placed to the line before http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@76 PS4, Line 76: TPlanNodeExecSummary& summary nit: we put output paramaters at the end of the parameter list, and also use pointer type, i.e. TPlanNodeExecSummary* http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@91 PS4, Line 91: * nit: please add spaces around the operator http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@86 PS4, Line 86: if (id % 2 == 0) { : node_summary.__set_is_broadcast(true); : } : addStatsToSummary(node_summary, id); : if (id % 3 == 0) { : addStatsToSummary(node_summary, 2*id); : } Please add comment what is the reasoning here http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@96 PS4, Line 96: TPlanFragment* fragment, TExecSummary* summary nit: output parameters should come last. This applies to all subsequent functions. http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@97 PS4, Line 97: string label, string label_detail The perf is not too relevant for this test, but these should typically be const string& http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@100 PS4, Line 100: nit: continuation lines need +4 spaces, not +2 http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@128 PS4, Line 128: TDataSink& sink nit: should it be const? http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@132 PS4, Line 132: static void prepareSelectStatement( I feel like that general purpose helper methods and methods required for certain tests are intermixed in this class. It would be easier to read the code if you could seperate the responsibilities. http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@139 PS4, Line 139: need +4, same goes for L141 http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@219 PS4, Line 219: needs +4 indent -- To view, visit http://gerrit.cloudera.org:8080/22799 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1b7f2b3a914c9091db51fbed117a80330f1d6fe5 Gerrit-Change-Number: 22799 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Vanko <dva...@cloudera.com> Gerrit-Reviewer: Daniel Vanko <dva...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Mon, 28 Apr 2025 16:19:41 +0000 Gerrit-HasComments: Yes
