Surya Hebbar has posted comments on this change. ( http://gerrit.cloudera.org:8080/23068 )
Change subject: IMPALA-13474: Support aggregated profile for webUI's query timeline ...................................................................... Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/23068/3/www/scripts/query_timeline/fragment_diagram.js File www/scripts/query_timeline/fragment_diagram.js: http://gerrit.cloudera.org:8080/#/c/23068/3/www/scripts/query_timeline/fragment_diagram.js@34 PS3, Line 34: export let legacy_profile = false; > Use profile_version from TRuntimeProfileTree instead, like I actually realize this, but for imported query profiles or older versions of query profiles JSON. This would not be exposed until later versions of impala. So, there is a need to recognize the profile version even without the "profile_version: being exposed. Also, with IMPALA-9846, I hope to tackle this moving forward. http://gerrit.cloudera.org:8080/#/c/23068/3/www/scripts/query_timeline/fragment_diagram.js@595 PS3, Line 595: export function setTimingDiagramDimensions(ignored_arg) { : // phashes header and timeticks footer with margin and border by considering the offset : page_additional_height = timing_diagram.offsetTop + (ROW_HEIGHT + MARGIN_HEADER_FOOTER : + BORDER_STROKE_WIDTH * 4) * 2; : const FRAGMENT_DIAGRAM_INITIAL_HEIGHT = window.innerHeight - page_additional_height; : const REMAINING_HEIGHT = FRAGMENT_DIAGRAM_INITIAL_HEIGHT - getUtilizationWrapperHeight() : - getFragmentMetricsWrapperHeight(); : const DISPLAY_HEIGHT = Math.max(DIAGRAM_MIN_HEIGHT, Math.min(REMAINING_HEIGHT, : rownum * ROW_HEIGHT)); : chart_width = diagram_width - name_width - MARGIN_CHART_END - BORDER_STROKE_WIDTH; : : phases_header.style.height = `${ROW_HEIGHT}px`; : fragment_diagram.parentElement.style.height = `${DISPLAY_HEIGHT}px`; : fragment_diagram.style.height = `${Math.max(DIAGRAM_MIN_HEIGHT, : rownum * ROW_HEIGHT)}px`; : timeticks_footer.style.height = `${ROW_HEIGHT}px`; : : fragment_diagram.parentElement.style.width = `${diagram_width}px`; : phases_header.parentElement.style.width = `${diagram_width}px`; : timeticks_footer.parentElement.style.width = `${diagram_width}px`; : timing_diagram.style.width = `${diagram_width}px`; : : fragment_diagram.style.width = `${diagram_width}px`; : phases_header.style.width = `${diagram_width}px`; : timeticks_footer.style.width = `${diagram_width}px`; : } : : export function renderTimingDiagram() { : if (fragment_events_parse_successful) { : setTimingDiagramDimensions(); : renderPhases(); : if (legacy_profile) { : renderFragmentDiagram_v1(); : } else { : renderFragmentDiagram(); : } : renderTimeticks(); : } : } > nit: is it necessary to move around functions from its previous locations? I had tried multiple ways of moving around the code. This was the reason for starting the current new change, as I abandoned the previous change. https://gerrit.cloudera.org/#/c/22043/ Many previous possible variations of moving around code sit in draft 3,4 changes of this previous patch. But, the current method seems to be the only correct way to arrange the code. Please let me know, if there is a different method, I should arrange the code other than these patchsets. http://gerrit.cloudera.org:8080/#/c/23068/3/www/scripts/query_timeline/fragment_diagram.js@720 PS3, Line 720: _v1(eve > Avoid naming with "legacy" suffix or prefix. Instead, use _v1 or _v2. Done. Thank you. -- To view, visit http://gerrit.cloudera.org:8080/23068 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If1f59bc8ab6bee91bd5fa446a207891453600b3e Gerrit-Change-Number: 23068 Gerrit-PatchSet: 4 Gerrit-Owner: Surya Hebbar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Tue, 08 Jul 2025 23:07:04 +0000 Gerrit-HasComments: Yes
