Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/23883 )
Change subject: IMPALA-572 impala-shell: add option to write profiles to a file ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/23883/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23883/3//COMMIT_MSG@13 PS3, Line 13: nit: use spaces instead of tabs http://gerrit.cloudera.org:8080/#/c/23883/3/shell/impala_shell/impala_shell.py File shell/impala_shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/23883/3/shell/impala_shell/impala_shell.py@1242 PS3, Line 1242: def profile_print(data): : if out_file: : out_file.write(data) : else: : print(data) It seems simpler to always use write or print and use sys.stdout in case there is no file provided. You are already using print with an output file in line 2477 Note that write and print has a the difference that print adds an extra \n at the end, so it should be out_file.write(data + "\n") to be equivalent. http://gerrit.cloudera.org:8080/#/c/23883/3/shell/impala_shell/option_parser.py File shell/impala_shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/23883/3/shell/impala_shell/option_parser.py@211 PS3, Line 211: "queries will be appended to the same file") nit: +1 space http://gerrit.cloudera.org:8080/#/c/23883/3/tests/shell/test_shell_commandline.py File tests/shell/test_shell_commandline.py: http://gerrit.cloudera.org:8080/#/c/23883/3/tests/shell/test_shell_commandline.py@427 PS3, Line 427: args = ['-q', 'select 1; profile;', '--profile_output=%s' % tmp_file] Can you also test the case when -p/show_profiles option is used? http://gerrit.cloudera.org:8080/#/c/23883/3/tests/shell/test_shell_commandline.py@434 PS3, Line 434: "Did not expect runtime profile in stdout, stdout: %s" % result_set.stdout nit: when breaking lines, we usually use 4 spaces indentation. e.g see line 417 http://gerrit.cloudera.org:8080/#/c/23883/3/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/23883/3/tests/shell/test_shell_interactive.py@247 PS3, Line 247: self._expect_with_cmd(proc, "set profile_output=/tmp/profile.txt", vector) It would be nice to test that the config is actually used. I expect something like: 1. set profile_output to a file 2. run query + profile; 3. check that it is written to a file 4. unset profile_output 5. run query + profile; 6. check that profile is written to stdout -- To view, visit http://gerrit.cloudera.org:8080/23883 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8ce4ddcf013392b3c4d66941f07fb90f9c90c3c Gerrit-Change-Number: 23883 Gerrit-PatchSet: 3 Gerrit-Owner: Balazs Hevele <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 22 Jan 2026 09:14:06 +0000 Gerrit-HasComments: Yes
