Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/24429 )
Change subject: IMPALA-15090: Make Query Profile Redaction DOM to DOM ...................................................................... Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/24429/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24429/3//COMMIT_MSG@12 PS3, Line 12: extra JsonToString() allocations in the AI profile-analysis flow. Please add information about how this change was tested. http://gerrit.cloudera.org:8080/#/c/24429/3/be/src/service/query-profile-ai-analysis.cc File be/src/service/query-profile-ai-analysis.cc: http://gerrit.cloudera.org:8080/#/c/24429/3/be/src/service/query-profile-ai-analysis.cc@935 PS3, Line 935: const size_t redacted_profile_size_bytes = redactor.redacted_profile_size_bytes(); redacted_profile_size_bytes is only used in one place (line 940). Eliminate this variable declaration and directly call redactor.redacted_profile_size_bytes() in the call to CreateQueryProfileToolExecutorForProfile(). http://gerrit.cloudera.org:8080/#/c/24429/3/be/src/service/query-profile-redaction.cc File be/src/service/query-profile-redaction.cc: http://gerrit.cloudera.org:8080/#/c/24429/3/be/src/service/query-profile-redaction.cc@332 PS3, Line 332: if (node.IsString()) { : values->emplace_back(node.GetString(), node.GetStringLength()); : return; : } : if (node.IsArray()) { : for (const auto& item : node.GetArray()) { : CollectStringValuesFromJsonImpl(item, values); : } : return; : } : if (!node.IsObject()) return; : for (auto it = node.MemberBegin(); it != node.MemberEnd(); ++it) { : CollectStringValuesFromJsonImpl(it->value, values); : } This code block is structured in a way that is somewhat confusing to understand because of all the early returns. Please structure it as an if () {} else if() {} else if() {} instead. http://gerrit.cloudera.org:8080/#/c/24429/3/be/src/service/query-profile-redaction.cc@349 PS3, Line 349: static vector<string_view> CollectStringValuesFromJson(const Value& node) { Please add a ctest test to cover this function. http://gerrit.cloudera.org:8080/#/c/24429/3/be/src/service/query-profile-redaction.cc@356 PS3, Line 356: static vector<string> CollectRegexMatchesFromTexts( Please add a ctest test to cover this function. http://gerrit.cloudera.org:8080/#/c/24429/3/be/src/service/query-profile-redaction.cc@371 PS3, Line 371: static vector<string> CollectIpv6MatchesFromTexts(const vector<string_view>& texts) { Please add a ctest test to cover this function. http://gerrit.cloudera.org:8080/#/c/24429/3/be/src/service/query-profile-redaction.cc@369 PS3, Line 369: : // Collects unique IPv6 matches across a sequence of text inputs. : static vector<string> CollectIpv6MatchesFromTexts(const vector<string_view>& texts) { : unordered_set<string> seen; : vector<string> results; : for (const string_view& text : texts) { : vector<string> matches = CollectIpv6Matches(text); : for (string& token : matches) { : if (!seen.emplace(token).second) continue; : results.emplace_back(move(token)); : } : } : return results; : } Can this whole function be replaced by calling CollectRegexMatchesFromTexts() with an IPv6 regex? http://gerrit.cloudera.org:8080/#/c/24429/3/be/src/service/query-profile-redaction.cc@384 PS3, Line 384: // Estimates serialized JSON size directly from a DOM value. Please clarify the unit of the returned size (bytes, kilobytes, etc). http://gerrit.cloudera.org:8080/#/c/24429/3/be/src/service/query-profile-redaction.cc@385 PS3, Line 385: static size_t EstimateSerializedJsonSize(const Value& value) { Please add a ctest test to cover this function. -- To view, visit http://gerrit.cloudera.org:8080/24429 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b8ad721a5531e9d257cf6e54b83ea3ee9734039 Gerrit-Change-Number: 24429 Gerrit-PatchSet: 3 Gerrit-Owner: Gokul Kolady <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Fri, 12 Jun 2026 18:32:53 +0000 Gerrit-HasComments: Yes
