Amogh Margoor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17855 )
Change subject: IMPALA-10921 Add script to compare TPCDS runs. ...................................................................... Patch Set 2: (28 comments) http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py File bin/diagnostics/experimental/tpcds_run_comparator.py: http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@44 PS1, Line 44: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@57 PS1, Line 57: RE_PEAK_MEM = re.compile("\d+\.\d\d [GMK]?B") > nit: could you please add some comments for SECTIONS? It's a bit misleading Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@72 PS1, Line 72: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@76 PS1, Line 76: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@79 PS1, Line 79: > flake8: E226 missing whitespace around arithmetic operator Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@81 PS1, Line 81: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@93 PS1, Line 93: unit = parts[1] > Maybe raise an error for unknown 'unit'. Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@96 PS1, Line 96: elif unit == 'KB': > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@105 PS1, Line 105: > flake8: E265 block comment should start with '# ' Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@106 PS1, Line 106: > flake8: E225 missing whitespace around operator Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@107 PS1, Line 107: : > flake8: E225 missing whitespace around operator Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@108 PS1, Line 108: """Class that parses Impala plain text query profile""" > line has trailing whitespace Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@108 PS1, Line 108: """Class that parses Impala plain text query profile""" > flake8: W293 blank line contains whitespace Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@110 PS1, Line 110: d > flake8: E303 too many blank lines (2) Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@132 PS1, Line 132: """Parse execution summary section. > Could you add some comments here? Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@180 PS1, Line 180: > maybe it would be interesting to see if there was an increase in some cases Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@181 PS1, Line 181: > flake8: E226 missing whitespace around arithmetic operator Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@184 PS1, Line 184: > flake8: E226 missing whitespace around arithmetic operator Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@184 PS1, Line 184: > flake8: E226 missing whitespace around arithmetic operator Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@185 PS1, Line 185: > flake8: E226 missing whitespace around arithmetic operator Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@192 PS1, Line 192: _ > flake8: E226 missing whitespace around arithmetic operator Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@211 PS1, Line 211: geo_mean2 = geo_mean(pp.peak_mem) > It will be great if we can add option to print this output to 2 csv files. Added the option to write results to csv. http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@227 PS1, Line 227: def print_results(ht_mem_res, op_mem_res): > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@231 PS1, Line 231: operator Peak Memory") > needs format? Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@232 PS1, Line 232: print("""TPCDS query profile, base peak memory, new peak memory""") > flake8: E302 expected 2 blank lines, found 0 Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@245 PS1, Line 245: with open(csv_path, 'w') as csv_f: : writer = csv.writer(csv_f) > For iterating the profiles, what if we just use simple filename matching be Good suggestion. taken care of. http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@248 PS1, Line 248: r > flake8: E226 missing whitespace around arithmetic operator Done http://gerrit.cloudera.org:8080/#/c/17855/1/bin/diagnostics/experimental/tpcds_run_comparator.py@262 PS1, Line 262: > flake8: E305 expected 2 blank lines after class or function definition, fou Done -- To view, visit http://gerrit.cloudera.org:8080/17855 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2e9ae1a2919156b0022072f47ff71d7775b20e6 Gerrit-Change-Number: 17855 Gerrit-PatchSet: 2 Gerrit-Owner: Amogh Margoor <amarg...@gmail.com> Gerrit-Reviewer: Amogh Margoor <amarg...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Mon, 11 Oct 2021 13:19:15 +0000 Gerrit-HasComments: Yes
