Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21619 )
Change subject: [tools] Update plan-graph.py ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/21619/1/bin/diagnostics/experimental/plan-graph.py File bin/diagnostics/experimental/plan-graph.py: http://gerrit.cloudera.org:8080/#/c/21619/1/bin/diagnostics/experimental/plan-graph.py@a155 PS1, Line 155: Removing the present tense doesn't read as an improvement to me. http://gerrit.cloudera.org:8080/#/c/21619/1/bin/diagnostics/experimental/plan-graph.py@a287 PS1, Line 287: nit: I wouldn't remove the 's'. If I run the new sentence through a grammar checker, it recommends adding back the 's'. http://gerrit.cloudera.org:8080/#/c/21619/1/bin/diagnostics/experimental/plan-graph.py@a431 PS1, Line 431: I don't think any of the changes to this sentence are necessary. http://gerrit.cloudera.org:8080/#/c/21619/1/bin/diagnostics/experimental/plan-graph.py@29 PS1, Line 29: # This script read Impala plain text query profile and output GraphViz DOT file Why the change in grammar? What was there before seemed more readable. http://gerrit.cloudera.org:8080/#/c/21619/1/bin/diagnostics/experimental/plan-graph.py@89 PS1, Line 89: # ParseTask(Task.DISCARD, Why is this commented out? http://gerrit.cloudera.org:8080/#/c/21619/1/bin/diagnostics/experimental/plan-graph.py@470 PS1, Line 470: if (row_actual > row_est * 50): Can you add a comment on why you chose 50x? http://gerrit.cloudera.org:8080/#/c/21619/1/bin/diagnostics/experimental/plan-graph.py@490 PS1, Line 490: attrib['color'] = CL_E_RF I'd suggest adding a helper method to set attrib['color'] when no_color is false. I see other places where you don't check no_color, and not sure if those are intentional or mistakes. -- To view, visit http://gerrit.cloudera.org:8080/21619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22b0188bba3eef120c3e4b0f48408088123c4650 Gerrit-Change-Number: 21619 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Wed, 31 Jul 2024 21:16:02 +0000 Gerrit-HasComments: Yes
