Surya Hebbar 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 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/query_timeline/fragment_diagram.js File www/scripts/query_timeline/fragment_diagram.js: http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/query_timeline/fragment_diagram.js@84 PS8, Line 84: // 'dasharray' and 'stroke_color' are optional parameters > nit: Can you specify default value in JS? Thanks for this suggestion. Now, I have made these 2 optional arguments. Though in JS there is no need to set 'undefined'. I had thought explicitly mentioning might be clear sometimes. But, this is better as it saves the trouble of writing redundant parameters. So, I am keeping like it previously was with comments about the optional parameters. http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/query_timeline/fragment_diagram.js@112 PS8, Line 112: > nit: max_width > 0 I have made it an optional parameter now, like previously. http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/query_timeline/fragment_diagram.js@199 PS8, Line 199: // } > (0,0) coordinate is top-left or bottom-left? Done http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/query_timeline/fragment_diagram.js@233 PS8, Line 233: Represent the aggregate distribution of instances for each phase > Move this comment inside parenthesis of L232. Done http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/query_timeline/fragment_diagram.js@239 PS8, Line 239: > Consider wrapping this into a function to ensure same formula at L239 and L Done http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/query_timeline/fragment_diagram.js@435 PS8, Line 435: row_heigh > nit: 0 Done http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/tests/query_timeline/fragment_diagram.test.js File www/scripts/tests/query_timeline/fragment_diagram.test.js: http://gerrit.cloudera.org:8080/#/c/21593/8/www/scripts/tests/query_timeline/fragment_diagram.test.js@52 PS8, Line 52: test("Test getSvgTitle", () => { : expect(getSvgTitle("Title").outerHTML).toBe("<title>Title</title>"); : }); : : test("Test getSvgGroup", () => { : expect(getSvgGroup().outerHTML).toBe("<g></g>"); : }); Removed the duplicate 'getSvgText' function test. -- 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: 9 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, 17 Sep 2024 21:05:14 +0000 Gerrit-HasComments: Yes
