Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/10900 )
Change subject: IMPALA-1624: Allow toggling some command-line options inside impala-shell ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/10900/1/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/10900/1/shell/impala_shell.py@135 PS1, Line 135: "true", "TRUE", "True", "1" This could be moved to a separate variable like TRUE_STRINGS or the whole logic could be moved to a function. http://gerrit.cloudera.org:8080/#/c/10900/1/shell/impala_shell.py@137 PS1, Line 137: \\s Will this handle other special characters like tabs and new lines? http://gerrit.cloudera.org:8080/#/c/10900/1/shell/impala_shell.py@138 PS1, Line 138: None I would prefer an empty string instead on 'None' to be consistent with the command line option (running the shell with argument "-o None" will create a file named None. http://gerrit.cloudera.org:8080/#/c/10900/1/shell/impala_shell.py@1435 PS1, Line 1435: , I have noticed that this tip is wrong. Please correct the default to \t. It could be also useful to add a tip for the new shell options. http://gerrit.cloudera.org:8080/#/c/10900/1/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/10900/1/tests/shell/test_shell_interactive.py@98 PS1, Line 98: assert "+----------------+" not in result.stdout Please also add a positive test (a row that should be included in the result). http://gerrit.cloudera.org:8080/#/c/10900/1/tests/shell/test_shell_interactive.py@119 PS1, Line 119: assert "ALGERIA" not in result.stdout It should be also verified that the results were written to the file. http://gerrit.cloudera.org:8080/#/c/10900/1/tests/shell/test_shell_interactive.py@125 PS1, Line 125: p2.send_cmd("set output_file=None") The primary way to set back options to their default should be calling "unset". This does not work for shell options at the moment, but is should be trivial to fix it (see IMPALA-7286). -- To view, visit http://gerrit.cloudera.org:8080/10900 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8d4487c24f24806223bfd5c54336914e3afd763 Gerrit-Change-Number: 10900 Gerrit-PatchSet: 1 Gerrit-Owner: Le Minh Nghia <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Jinchul Kim <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 12 Jul 2018 12:42:23 +0000 Gerrit-HasComments: Yes
