[Impala-ASF-CR] IMPALA-1624: Allow toggling some command-line options inside impala-shell
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 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/10900/2/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/10900/2/shell/impala_shell.py@131
PS2, Line 131: # Valid true value for some shell options
nit: we typically finish comments with '.' in Impala
I would also rephrase the sentence to something like "Strings that are
interpreted as True for some boolean options."
http://gerrit.cloudera.org:8080/#/c/10900/2/shell/impala_shell.py@177
PS2, Line 177: \
nit: please add a space before the last \ - I think that we do this everywhere
in Impala.
http://gerrit.cloudera.org:8080/#/c/10900/2/shell/impala_shell.py@743
PS2, Line 743: elif not self._handle_unset_shell_options(option):
Please print a message when the unset succeeds.
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@125
PS1, Line 125: p2 = ImpalaShell()
> Done
Thanks for doing IMPALA-7286! Please also mention this change in the commit
message.
http://gerrit.cloudera.org:8080/#/c/10900/2/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:
http://gerrit.cloudera.org:8080/#/c/10900/2/tests/shell/test_shell_interactive.py@94
PS2, Line 94: p.send_cmd("set delimiter=,")
Can we keep the default delimiter in this test to cover more scenarios by the
tests?
--
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: 2
Gerrit-Owner: Le Minh Nghia
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Jinchul Kim
Gerrit-Reviewer: Le Minh Nghia
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Fri, 13 Jul 2018 15:46:02 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1624: Allow toggling some command-line options inside impala-shell
Le Minh Nghia 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 2: (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: alaShell.TRUE_STRINGS, "pri > This could be moved to a separate variable like TRUE_STRINGS or the whole l Done http://gerrit.cloudera.org:8080/#/c/10900/1/shell/impala_shell.py@137 PS1, Line 137: pal > Will this handle other special characters like tabs and new lines? Yes, it will. http://gerrit.cloudera.org:8080/#/c/10900/1/shell/impala_shell.py@138 PS1, Line 138: RUE_ > I would prefer an empty string instead on 'None' to be consistent with the Done 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 Done 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 "21,VIETNAM,2" in result.stdout > Please also add a positive test (a row that should be included in the resul Done http://gerrit.cloudera.org:8080/#/c/10900/1/tests/shell/test_shell_interactive.py@119 PS1, Line 119: assert "VIETNAM" not in result.stdout > It should be also verified that the results were written to the file. Done http://gerrit.cloudera.org:8080/#/c/10900/1/tests/shell/test_shell_interactive.py@125 PS1, Line 125: p2 = ImpalaShell() > The primary way to set back options to their default should be calling "uns Done -- 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: 2 Gerrit-Owner: Le Minh Nghia Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Jinchul Kim Gerrit-Reviewer: Le Minh Nghia Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 13 Jul 2018 15:20:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1624: Allow toggling some command-line options inside impala-shell
Hello Zoltan Borok-Nagy, Csaba Ringhofer, Jinchul Kim, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10900 to look at the new patch set (#2). Change subject: IMPALA-1624: Allow toggling some command-line options inside impala-shell .. IMPALA-1624: Allow toggling some command-line options inside impala-shell This change provides a way to modify command-line options like -B, --output_file and --delimiter inside impala-shell without quitting the shell and then restarting again. Testing: Added tests for all new options in test_shell_interactive.py Change-Id: Id8d4487c24f24806223bfd5c54336914e3afd763 --- M shell/impala_shell.py M tests/shell/test_shell_interactive.py 2 files changed, 72 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/10900/2 -- 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: newpatchset Gerrit-Change-Id: Id8d4487c24f24806223bfd5c54336914e3afd763 Gerrit-Change-Number: 10900 Gerrit-PatchSet: 2 Gerrit-Owner: Le Minh Nghia Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Jinchul Kim Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-1624: Allow toggling some command-line options inside impala-shell
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
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Jinchul Kim
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Thu, 12 Jul 2018 12:42:23 +
Gerrit-HasComments: Yes
