Daniel Vanko has posted comments on this change. ( http://gerrit.cloudera.org:8080/22799 )
Change subject: IMPALA-13824: add unit tests for PlanToJson ...................................................................... Patch Set 5: (14 comments) 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 it's needed for the tests, because they're not in impala namespace http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@35 PS4, Line 35: node_i > In C++, for variable names we don't use camelCase, we use underscores, i.e. Done http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@57 PS4, Line 57: st > nit: parameter lines need +4 spaces, not +2 Done http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@58 PS4, Line 58: const > nit: could be const? Done http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@61 PS4, Line 61: n > nit: this could be placed to the line before Done http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@76 PS4, Line 76: eline + 1) * 2); > nit: we put output paramaters at the end of the parameter list, and also us Done 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 Done http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@86 PS4, Line 86: node_summary.__set_is_broadcast(true); : } : addStatsToSummary(id, &node_summary); : if (id % 3 == 0) { : addStatsToSummary(2 * id, &node_summary); : } : r > Please add comment what is the reasoning here Done http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@97 PS4, Line 97: (fragment->__isset.plan) { > The perf is not too relevant for this test, but these should typically be c Done http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@100 PS4, Line 100: el > nit: continuation lines need +4 spaces, not +2 Done http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@128 PS4, Line 128: > nit: should it be const? Done http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@132 PS4, Line 132: > I feel like that general purpose helper methods and methods required for ce I put it outside of the class for now http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@139 PS4, Line 139: sc > need +4, same goes for L141 Done http://gerrit.cloudera.org:8080/#/c/22799/4/be/src/service/impala-http-handler-test.cc@219 PS4, Line 219: > needs +4 indent Done -- 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: 5 Gerrit-Owner: Daniel Vanko <[email protected]> Gerrit-Reviewer: Daniel Vanko <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 28 May 2025 15:19:55 +0000 Gerrit-HasComments: Yes
