impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2640: Make a given command case-sensitive
Date Tue, 12 Dec 2017 23:31:06 GMT
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8762 )

Change subject: IMPALA-2640: Make a given command case-sensitive
......................................................................


Patch Set 6:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8762/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8762/6//COMMIT_MSG@7
PS6, Line 7: IMPALA-2640: Make a given command case-sensitive
Unless I'm missing something, doesn't this solve IMPALA-4664 too? I checked out this commit
and played around and I get:

[localhost:21000] > sElect'FOO';
Query: sElect 'FOO'
Query submitted at: 2017-12-12 15:30:44 (Coordinator: http://tarmstrong-box:25000)
Query progress can be monitored at: http://tarmstrong-box:25000/query_plan?query_id=c049cc68f0842616:4419587c00000000
+-------+
| 'foo' |
+-------+
| FOO   |
+-------+
Fetched 1 row(s) in 0.00s


http://gerrit.cloudera.org:8080/#/c/8762/6/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8762/6/shell/impala_shell.py@572
PS6, Line 572: return func(arg, cmd_)
> Done.
It's a matter of taste but I think passing in cmd as an argument is clearer - fewer member
variables and implicit argument parsing seems easier to follow. I agree it still feels a bit
weird in some ways. Anyway, I think I'm ok with the current version.


http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@305
PS9, Line 305:                                                  self.set_query_options)
Long line


http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@310
PS9, Line 310:            ! <cmd>
Long line.


http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@555
PS9, Line 555: 
Can you include attribution for where the copied code came from (e.g. see be/src/runtime/string-search.h).
It also needs to be added to LICENSE.txt. It looks like it's licensed with the PSF license
V2, which we already include in LICENSE.txt.


http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@571
PS9, Line 571: etur
_ on the end of a local variable is a bit weird to me - can we call it command instead?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul <jinchul@gmail.com>
Gerrit-Reviewer: Andre Araujo <araujo@cloudera.com>
Gerrit-Reviewer: John Russell <jrussell@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <jinchul@gmail.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <boroknagyz@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2017 23:31:06 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message