Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21593 )
Change subject: IMPALA-13233: Improve display of instance-level skew in query timeline ...................................................................... Patch Set 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js File www/scripts/query_timeline/fragment_diagram.js: http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js@86 PS5, Line 86: if (stroke_color) { : rect.setAttribute("stroke", stroke_color); : } : if (dasharray) { : rect.setAttribute("stroke-dasharray", dasharray); : } stroke_color and dasharray argument is called inconsistently. One time it accept string, and other time it accept boolean False. Maybe it is better to always have these attributes and let caller set them to "none" when they don't actually want them? https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/stroke#usage_notes https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/stroke-dasharray#usage_notes http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js@95 PS5, Line 95: max_width Please consistently pass integer rather than boolean False for max_width. http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js@194 PS5, Line 194: var y = rownum * row_height; : var cp_y = y; : var dx, dy; I'm rereading this code and confused again what do these variables means. I assume y is y-axis, but there is no variable x (or is it suppose to be xoffset? maybe yoffset is better then?). Is (0,0) coordinate in top-left or bottom-left? Is dx and dy increments of x and y coordinate as you draw the SVG rectangle? I confuse it with dx/dy from calculus term. Should the initialization like following instead? var cp_y = rownum * row_height; var y = cp_y; Because c_y never reassigned, while y reinitialized with cp_y again in L216 and L251. And what is cp_y means? Please add comment describing how this function works. http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js@199 PS5, Line 199: 5 This should be a constant like MAX_BUCKET, and reference to last index like in L257 should use [MAX_BUCKET - 1] http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js@223 PS5, Line 223: if (part.count > 0) dy = (part.count / events[i].ts_list.length) * bar_height; : else continue; Use parentheses for clarity. Please add comment why continue if part.count <= 0. http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js@225 PS5, Line 225: if (dy < 1) dy = 1; What is the significance of dy = 1? How can dy be < 1 in the first place? http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js@227 PS5, Line 227: plan_node.appendChild(attachBucketedPhaseTooltip(getSvgRect( : phases[color_idx].color, xoffset, y, dx, dy, : `0 ${l_dx} ${dx - l_dx + dy} ${dx} ${dy} 0`, : stroke_fill_colors.black), part)); This is too long to write in single statement. Consider breaking this down with variable. var rect = getSvgRect(... rect = attachBucketedPhaseTooltip(rect,... plan_node.appendChild(rect); http://gerrit.cloudera.org:8080/#/c/21593/5/www/scripts/query_timeline/fragment_diagram.js@241 PS5, Line 241: plan_node.appendChild(getSvgRect(phases[color_idx].color, xoffset, y, dx, dy, : `0 ${l_dx} ${dx - l_dx + dy} ${dx} ${dy} 0`, stroke_fill_colors.black)); Consider breaking this down with variable. -- 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: 5 Gerrit-Owner: Surya Hebbar <sheb...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Surya Hebbar <sheb...@cloudera.com> Gerrit-Comment-Date: Wed, 04 Sep 2024 18:19:42 +0000 Gerrit-HasComments: Yes
