Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/24576 )
Change subject: IMPALA-15174: Add AGGREGATED_PROFILE query option to toggle profile formats ...................................................................... Patch Set 2: (13 comments) http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.h@310 PS2, Line 310: aggregated_profile members should end with _ http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.h@538 PS2, Line 538: aggregated_profile members should end with _ http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.cc@61 PS2, Line 61: aggregated_profile This doesn't seem to be used in the file http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.cc@515 PS2, Line 515: exec_params_.query_options().aggregated_profile Could use a local "bool aggregated_profile" for this, it is used at several places. http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.cc@517 PS2, Line 517: aggregated_profile this indicates flag instead of query option http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.cc@533 PS2, Line 533: aggregated_profile same as line 517 http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator-backend-state.cc@636 PS2, Line 636: experimental Is it still experimental? http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator.cc@84 PS2, Line 84: DECLARE_bool(aggregated_profile); seems unused http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/runtime/coordinator.cc@256 PS2, Line 256: query_profile_->AddInfoString("Profile Type", : static_cast<stringstream&>(stringstream() << profile_type).str()); I don't understand this, why it is added in a loop that iterates through fragments? Each fragment should use the same profile type http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/service/impala-server.cc@997 PS2, Line 997: const::string* profile_type = exec_profile->GetInfoString("Profile Type"); Can't QueryStateRecord have this information? Its constructors get ClientRequestState, which has query options, so it QueryStateRecord could set a member during construction http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/service/impala-server.cc@2186 PS2, Line 2186: default_query_options_.__set_aggregated_profile(FLAGS_aggregated_profile); Is it useful to have both flag and query option? Users could override in FLAGS_default_query_options. The flags set both ways above didn't have a query option in the past, the flag is kept for backward compatibility. I am not sure here, maybe for such an important option having an extra flag can be useful to avoid noise in default_qeury_options. http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/util/impala-profile-tool.cc File be/src/util/impala-profile-tool.cc: http://gerrit.cloudera.org:8080/#/c/24576/2/be/src/util/impala-profile-tool.cc@a138 PS2, Line 138: Why is this no longer used? http://gerrit.cloudera.org:8080/#/c/24576/2/tests/custom_cluster/test_observability.py File tests/custom_cluster/test_observability.py: http://gerrit.cloudera.org:8080/#/c/24576/2/tests/custom_cluster/test_observability.py@100 PS2, Line 100: ggregated_profile This could be moved out from this suite to a non-custom cluster test (probably in a follow-up commit). -- To view, visit http://gerrit.cloudera.org:8080/24576 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I96aceda1b044ae53d09e68cb78678fc2930326dd Gerrit-Change-Number: 24576 Gerrit-PatchSet: 2 Gerrit-Owner: Surya Hebbar <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Wed, 22 Jul 2026 15:57:42 +0000 Gerrit-HasComments: Yes
