impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tianyi Wang (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-992: Rerun past queries from history in shell
Date Tue, 15 Aug 2017 21:36:35 GMT
Tianyi Wang has posted comments on this change.

Change subject: IMPALA-992: Rerun past queries from history in shell
......................................................................


Patch Set 4:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/7674/2//COMMIT_MSG
Commit Message:

PS2, Line 7: IMPALA
> nit: capitalize
Done


PS2, Line 9: @
> What's the reason for picking "@" over "!"? The latter is used in shells an
Currently "!" is used to run shell commands in impala-shell.


PS2, Line 12: _length, -1]. Negavie val
> Can you give a usage example? Does the history count backwards or forwards 
More example and explanation is given.


http://gerrit.cloudera.org:8080/#/c/7674/2/shell/impala_shell.py
File shell/impala_shell.py:

PS2, Line 79: object
> Why is this needed?
The direct reason is that "super" in line 1164 cannot be used with an old style class. And
old style class is not recommended after python 2.1.
https://stackoverflow.com/questions/13816849/old-style-and-new-style-classes-in-python-2-7


Line 1073:     print_to_stderr("The readline module was either not found or disabled. Command
"
> nit: You can remove the else and unindent the block, just like you do in th
Done


PS2, Line 1080: one' a
> Not your change, but:
Done


PS2, Line 1090: 
> Format on two lines to make it easier to read.
Done


PS2, Line 1102:     if not (0 < cmd_idx <= history_length or -history_length <= cmd_idx
< 0):
> Please use explicit indexing or kwarg-style format: "{}".format(str) is not
Done


PS2, Line 1102:     if not (0 < cmd_idx <= history_length or -history_length <= cmd_idx
< 0):
> long line
Done


Line 1107:       cmd_idx += history_length + 1
> Does it always have a semicolon? .rstrip(";") might be more robust and clea
Done


Line 1156:         # The history file is not writable, disable readline.
> Can you mention that this method will perform transformations?
Done


PS2, Line 1157: 
> what will 'line' contain?
One more line of comment is added.


-- 
To view, visit http://gerrit.cloudera.org:8080/7674
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc28e8ce07845343267224c3b9ccb71b29a524d2
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang <twang@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <twang@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message