Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21593 )
Change subject: IMPALA-13233: Represent phases of each instance separately on the query timeline ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/21593/3/www/scripts/query_timeline/chart_commons.js File www/scripts/query_timeline/chart_commons.js: http://gerrit.cloudera.org:8080/#/c/21593/3/www/scripts/query_timeline/chart_commons.js@101 PS3, Line 101: chart === null What is the difference between null vs undefined here? http://gerrit.cloudera.org:8080/#/c/21593/3/www/scripts/query_timeline/fragment_diagram.js File www/scripts/query_timeline/fragment_diagram.js: http://gerrit.cloudera.org:8080/#/c/21593/3/www/scripts/query_timeline/fragment_diagram.js@137 PS3, Line 137: if (ev.no_bar !== undefined) return; Please put another check for ev.tslist.length here, and return early if it is less than 4. http://gerrit.cloudera.org:8080/#/c/21593/3/www/scripts/query_timeline/fragment_diagram.js@148 PS3, Line 148: min = ev.tslist[k], max = ev.tslist[k], avg = 0; Separate each assignment in different lines. http://gerrit.cloudera.org:8080/#/c/21593/3/www/scripts/query_timeline/fragment_diagram.js@170 PS3, Line 170: no_bar : < Boolean value is set, if event is not to be rendered > I see this being compared against 'undefined' in multiple places. Is it because no_bar is not initialized in the beginning? Can you always initialize no_bar with true or false instead? http://gerrit.cloudera.org:8080/#/c/21593/3/www/scripts/query_timeline/fragment_diagram.js@171 PS3, Line 171: tslist : < List of this plan node's event's timestamps from all instances > Is this all timestamp from all instances for all phases, or just 1 phase (Prepare, Open, etc)? Is the element a type of TEventSequence, or TUnit, or i64? if i64, what is the unit (ns or ms)? nit: ts_list feels like a better name. http://gerrit.cloudera.org:8080/#/c/21593/3/www/scripts/query_timeline/fragment_diagram.js@197 PS3, Line 197: if (events[last_e_index].no_bar === undefined) { : // Plan node's top and bottom outlines : var top_edge_ts, bottom_edge_ts; : if (bucketed) { : top_edge_ts = events[last_e_index].parts[0].avg; : bottom_edge_ts = events[last_e_index].parts[3].avg; : } else { : top_edge_ts = events[last_e_index].tslist[0]; : bottom_edge_ts = events[last_e_index].tslist[last_ts_index]; : } : plan_node.appendChild(getSvgLine(stroke_fill_colors.black, xoffset, y, : xoffset + top_edge_ts * px_per_ns, y, false)); : plan_node.appendChild(getSvgLine(stroke_fill_colors.black, xoffset, y + bar_height, : xoffset + bottom_edge_ts * px_per_ns, y + bar_height, : false)); : } : : if (events[0].no_bar === undefined) { : if (bucketed) { : // Represent phases of each instance seperately, according to the timestamps : dy = bar_height / 4; : events[0].parts.forEach(function(part) { : dx = part.avg * px_per_ns; : // Don't print tiny skews : if (dx > ignore_px) { : // Add separators at the perimeters of each instance's phases : // through 'stroke-dasharray' property : plan_node.appendChild(attachBucketedPhaseTooltip(getSvgRect( : phases[color_idx].color, xoffset, y, dx, dy, `0 ${dx} ${dy} ${dx} ${dy} 0`, : stroke_fill_colors.black), part)); : } : y += dy; : }); : } else { : // Represent phases of each instance seperately, according to the timestamps : dy = bar_height / events[0].tslist.length; : events[0].tslist.forEach(function(ts) { : dx = ts * px_per_ns; : // Don't print tiny skews : if (dx > ignore_px) { : // Add separators at the perimeters of each instance's phases : // through 'stroke-dasharray' property : plan_node.appendChild(getSvgRect(phases[color_idx].color, xoffset, y, dx, dy, : `0 ${dx} ${dy} ${dx} ${dy} 0`, stroke_fill_colors.black)); : } : y += dy; : }); : } : ++color_idx; : } : var t; : var cp_i; : var cp_e; : for (var i = 1; i <= last_e_index; i++) { : y = rownum * row_height; : if (events[i].no_bar === undefined) { : cp_i = i; : while (events[--cp_i].no_bar !== undefined); : cp_e = events[cp_i]; : if (bucketed) { : // Represent phases of each instance seperately, according to the timestamps : dy = bar_height / 4; : events[i].parts.forEach(function(part, k) { : t = cp_e.parts[k].avg; : dx = Math.abs(part.avg - t) * px_per_ns; : // Don't print tiny skews : if (dx > ignore_px) { : // Add separators at the perimeters of each instance's phases : // through 'stroke-dasharray' property : plan_node.appendChild(attachBucketedPhaseTooltip(getSvgRect( : phases[color_idx].color, xoffset + t * px_per_ns, y, dx, dy, : `0 ${dx} ${dy} ${dx} ${dy} 0`,stroke_fill_colors.black), part)); : } : y += dy; : }); : } else { : // Represent phases of each instance seperately, according to the timestamps : dy = bar_height / events[i].tslist.length; : events[i].tslist.forEach(function(ts, k) { : t = cp_e.tslist[k]; : dx = Math.abs(ts - t) * px_per_ns; : // Don't print tiny skews : if (dx > ignore_px) { : // Add separators at the perimeters of each instance's phases : // through 'stroke-dasharray' property : plan_node.appendChild(getSvgRect(phases[color_idx].color, : xoffset + t * px_per_ns, y, dx, dy, `0 ${dx} ${dy} ${dx} ${dy} 0`, : stroke_fill_colors.black)); : } : y += dy; : }); : } : ++color_idx; : } : } Please break this down into two function: one for bucketed == true, and one for bucketed == false. http://gerrit.cloudera.org:8080/#/c/21593/3/www/scripts/query_timeline/fragment_diagram.js@522 PS3, Line 522: if (pp.node_metadata !== undefined) Can change to early return? http://gerrit.cloudera.org:8080/#/c/21593/3/www/scripts/query_timeline/fragment_diagram.js@524 PS3, Line 524: var name_flds = pp.profile_name.split(/[()]/); : var node_type = name_flds[0].trim(); : var node_id = name_flds.length > 1 ? name_flds[1].split(/[=]/)[1] : 0; Please put an example of pp.profile_name, name_flds, node_type, and node_id, as a comment. Why node_id seems to be string instead of integer (see comparison in L623)? http://gerrit.cloudera.org:8080/#/c/21593/3/www/scripts/query_timeline/fragment_diagram.js@538 PS3, Line 538: node_type === "EXCHANGE_NODE" Is strict equality necessary for string type? Same question for other string equality comparison. http://gerrit.cloudera.org:8080/#/c/21593/3/www/scripts/query_timeline/fragment_diagram.js@544 PS3, Line 544: if (node_type === "PLAN_ROOT_SINK") { : parent_node = undefined; : } Is this necessary if parent_node never initialized with any value in L543? http://gerrit.cloudera.org:8080/#/c/21593/3/www/scripts/query_timeline/fragment_diagram.js@548 PS3, Line 548: node_metadata.join_build_id TRuntimeProfileNodeMetadata does not seem to have join_build_id field. Can you double check please? -- To view, visit http://gerrit.cloudera.org:8080/21593 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ied8a5966e9e4111bf7aa25aee11d23881daad7d2 Gerrit-Change-Number: 21593 Gerrit-PatchSet: 3 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, 06 Aug 2024 17:12:02 +0000 Gerrit-HasComments: Yes
