[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8762 )

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Patch Set 16:

@Zoltan, Tim, I appreciate your review!


--
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: 16
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Sat, 16 Dec 2017 07:19:52 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8762 )

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..

IMPALA-4664: Unexpected string conversion in Shell

Impala shell can accidentally convert certain
literal strings to lowercase. Impala shell splits
each command into tokens and then converts the
first token to lowercase to figure out how it
should execute the command. The splitting is done
by spaces only. Thus, if the user types a TAB
after the SELECT, the first token after the split
becomes the SELECT plus whatever comes after it.

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShellInteractive.test_unexpected_conversion_for_literal_string_to_lowercase
TestImpalaShell.test_var_substitution

Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Reviewed-on: http://gerrit.cloudera.org:8080/8762
Reviewed-by: Zoltan Borok-Nagy 
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M LICENSE.txt
M bin/rat_exclude_files.txt
M shell/impala_shell.py
A tests/shell/shell_case_sensitive.cmds
A tests/shell/shell_case_sensitive2.cmds
M tests/shell/test_shell_interactive.py
6 files changed, 118 insertions(+), 49 deletions(-)

Approvals:
  Zoltan Borok-Nagy: Looks good to me, but someone else must approve
  Tim Armstrong: Looks good to me, approved
  Impala Public Jenkins: Verified

--
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: merged
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 16
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8762 )

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Patch Set 15: Verified+1


--
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: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 15 Dec 2017 21:32:19 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8762 )

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Patch Set 15:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1626/


--
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: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 15 Dec 2017 17:57:09 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8762 )

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Patch Set 15: Code-Review+2


--
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: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 15 Dec 2017 17:56:56 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8762 )

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Patch Set 15:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8762/15/bin/rat_exclude_files.txt
File bin/rat_exclude_files.txt:

http://gerrit.cloudera.org:8080/#/c/8762/15/bin/rat_exclude_files.txt@120
PS15, Line 120: tests/shell/shell_case_sensitive.cmds
  : tests/shell/shell_case_sensitive2.cmds
Fix for the failure:
+ bin/check-rat-report.py bin/rat_exclude_files.txt rat.xml
UNAPPROVED: tests/shell/shell_case_sensitive.cmds
UNAPPROVED: tests/shell/shell_case_sensitive2.cmds
(https://jenkins.impala.io/job/rat-check-ub1604/359/consoleText)


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

http://gerrit.cloudera.org:8080/#/c/8762/15/shell/impala_shell.py@1144
PS15, Line 1144: self.orig_cmd = "describe"
Fix for the failure:
E   desc test_do_methods_639a0d4c.test_do_methods
E   ^
E   Encountered: DESC
E   Expected: ALTER, COMPUTE, CREATE, DELETE, DESCRIBE, DROP, EXPLAIN, GRANT, 
INSERT, INVALIDATE, LOAD, REFRESH, REVOKE, SELECT, SET, SHOW, TRUNCATE, UPDATE, 
UPSERT, USE, VALUES, WITH
(https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/810)



--
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: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 15 Dec 2017 05:27:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Kim Jin Chul (Code Review)]

Hello John Russell, Andre Araujo, Zoltan Borok-Nagy, Tim Armstrong, Impala 
Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8762

to look at the new patch set (#14).

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..

IMPALA-4664: Unexpected string conversion in Shell

Impala shell can accidentally convert certain
literal strings to lowercase. Impala shell splits
each command into tokens and then converts the
first token to lowercase to figure out how it
should execute the command. The splitting is done
by spaces only. Thus, if the user types a TAB
after the SELECT, the first token after the split
becomes the SELECT plus whatever comes after it.

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShellInteractive.test_unexpected_conversion_for_literal_string_to_lowercase
TestImpalaShell.test_var_substitution

Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
---
M LICENSE.txt
M bin/rat_exclude_files.txt
M shell/impala_shell.py
A tests/shell/shell_case_sensitive.cmds
A tests/shell/shell_case_sensitive2.cmds
M tests/shell/test_shell_interactive.py
6 files changed, 118 insertions(+), 49 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/8762/14
--
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: newpatchset
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 14
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Kim Jin Chul (Code Review)]

Hello John Russell, Andre Araujo, Zoltan Borok-Nagy, Tim Armstrong, Impala 
Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8762

to look at the new patch set (#13).

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..

IMPALA-4664: Unexpected string conversion in Shell

Impala shell can accidentally convert certain
literal strings to lowercase. Impala shell splits
each command into tokens and then converts the
first token to lowercase to figure out how it
should execute the command. The splitting is done
by spaces only. Thus, if the user types a TAB
after the SELECT, the first token after the split
becomes the SELECT plus whatever comes after it.

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShellInteractive.test_unexpected_conversion_for_literal_string_to_lowercase
TestImpalaShell.test_var_substitution

Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
---
M LICENSE.txt
M shell/impala_shell.py
A tests/shell/shell_case_sensitive.cmds
A tests/shell/shell_case_sensitive2.cmds
M tests/shell/test_shell_interactive.py
5 files changed, 116 insertions(+), 49 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/8762/13
--
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: newpatchset
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 13
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8762 )

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Patch Set 12: Code-Review-1

Let me take a look at the failure and provide a new patch set.


--
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: 12
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 15 Dec 2017 02:23:16 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8762 )

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Patch Set 12: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1621/


--
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: 12
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 15 Dec 2017 02:12:09 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8762 )

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Patch Set 11: Code-Review+2


--
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: 11
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 14 Dec 2017 22:39:32 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Kim Jin Chul (Code Review)]

Hello John Russell, Andre Araujo, Zoltan Borok-Nagy, Tim Armstrong,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8762

to look at the new patch set (#11).

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..

IMPALA-4664: Unexpected string conversion in Shell

Impala shell can accidentally convert certain
literal strings to lowercase. Impala shell splits
each command into tokens and then converts the
first token to lowercase to figure out how it
should execute the command. The splitting is done
by spaces only. Thus, if the user types a TAB
after the SELECT, the first token after the split
becomes the SELECT plus whatever comes after it.

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShellInteractive.test_unexpected_conversion_for_literal_string_to_lowercase
TestImpalaShell.test_var_substitution

Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
---
M LICENSE.txt
M shell/impala_shell.py
A tests/shell/shell_case_sensitive.cmds
A tests/shell/shell_case_sensitive2.cmds
M tests/shell/test_shell_interactive.py
5 files changed, 112 insertions(+), 49 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/8762/11
--
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: newpatchset
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 11
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Kim Jin Chul (Code Review)]

Kim Jin Chul has abandoned this change. ( http://gerrit.cloudera.org:8080/8639 )

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Abandoned

The problem in IMPALA-4664 should be also resolved by the change: 
https://gerrit.cloudera.org/#/c/8762
--
To view, visit http://gerrit.cloudera.org:8080/8639
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Iba6fa4e9ac570d22135ba51b844db8f9be965ca3
Gerrit-Change-Number: 8639
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8639 )

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8639/4/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8639/4/shell/impala_shell.py@337
PS4, Line 337:   def sanitise_input(self, args):
> > I feel like all this string manipulation is very hard to reason about, le
I agree it would be nice to have a minimal fix for the bug but I think using 
sqlparse here is a major change. The code is confusing enough now that it's 
hard to understand the consequences of making the change.

I looked at the history of why we lower-case the first word. It seems like 
python cmd doesn't support case-insensitive routing of commands (e.g. "SeT 
foo=bar" -> do_set()).
See https://docs.python.org/2/library/cmd.html . The lower-casing is a hack to 
try and force python to do what we want. I think the proper fix is to undo this 
hack and fix it properly by overriding default() and doing the dispatching 
ourselves.

A possible minimal fix is to only lower-case the initial alphabetical 
characters in the input, instead of the whole first token. Python cmd only uses 
the initial alphanumeric characters to dispatch to a function: 
https://github.com/python/cpython/blob/2.7/Lib/cmd.py#L192



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba6fa4e9ac570d22135ba51b844db8f9be965ca3
Gerrit-Change-Number: 8639
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Mon, 11 Dec 2017 23:32:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8639 )

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8639/4/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8639/4/shell/impala_shell.py@337
PS4, Line 337:   def sanitise_input(self, args):
> I feel like all this string manipulation is very hard to reason about, 
> leading to bugs like this.

I agree. The current string manipulation logic has the following problems.
1. May have hidden bugs
2. Code maintenance is not easy because it has some special handling such as 
making lower-case command(IMPALA-2640)
3. Not to readable

Currently I would like to fix the problem with a minimal change. You may feel 
there is inefficient code. We should need to clarify the logic on an another 
JIRA ticket.

> It seems like if we're calling sqlparse.parse() to construct Statement 
> instances, we should return those Statements, which will be easier to operate 
> on, instead of converting it back to strings.
Instead we're translating tokens back to strings, joining then, then 
resplitting them with sqlparse.split().

sqlparse.parse() can return multiple statements.
(e.g. select 'foo'; select 'boo';)
Actually, I would like to rely on sqlparse.parse because it is used widely and 
confirmed by many users than our own parser logic. By the way, I am not sure 
the parse can work for all the Impala syntax. As far as I guarantee, it can 
parse command appropriately. If we will adopt the parse function widely, I need 
to get an answer for the following curiosity. You know SQL syntax in DBMSes and 
sql-on-hadoop(e.g. Impala, Hive) are slightly different each other. The systems 
try to follow up SQL standard, but, in fact, there are own specialized syntax 
on each system.

Here is my assumption. I guess we need to customize sqlparse to consume Impala 
syntax and sqlparse can recognize Impala's version to decide a set of available 
syntax. sqlparse can take some arguments a system name and a version to apply 
custom syntax. I am not sure this feature is already in sqlparse, but this is 
necessary to push upstream.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba6fa4e9ac570d22135ba51b844db8f9be965ca3
Gerrit-Change-Number: 8639
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Thu, 07 Dec 2017 02:11:56 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Kim Jin Chul (Code Review)]

Hello Tim Armstrong, Zach Amsden,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8639

to look at the new patch set (#4).

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..

IMPALA-4664: Unexpected string conversion in Shell

Impala shell can accidentally convert certain
literal strings to lowercase. Impala shell splits
each command into tokens and then converts the
first token to lowercase to figure out how it
should execute the command. The splitting is done
by spaces only. Thus, if the user types a TAB
after the SELECT, the first token after the split
becomes the SELECT plus whatever comes after it.

sqlparse libraries is upgraded from 0.1.14 to
0.2.4 due to bug fixes and syntax extension.
sqlparse 0.1.14 cannot parse assignment properly.
(e.g. set variable=value)

Testing:
Add tests to tests/shell/test_shell_interactive.py

Change-Id: Iba6fa4e9ac570d22135ba51b844db8f9be965ca3
---
M LICENSE.txt
M README.md
M bin/rat_exclude_files.txt
M bin/set-pythonpath.sh
M shell/.gitignore
R shell/ext-py/egg/prettytable-0.7.1/CHANGELOG
R shell/ext-py/egg/prettytable-0.7.1/COPYING
R shell/ext-py/egg/prettytable-0.7.1/MANIFEST.in
R shell/ext-py/egg/prettytable-0.7.1/PKG-INFO
R shell/ext-py/egg/prettytable-0.7.1/README
R shell/ext-py/egg/prettytable-0.7.1/prettytable.py
R shell/ext-py/egg/prettytable-0.7.1/setup.cfg
R shell/ext-py/egg/prettytable-0.7.1/setup.py
R shell/ext-py/egg/sasl-0.1.1/MANIFEST.in
R shell/ext-py/egg/sasl-0.1.1/PKG-INFO
R shell/ext-py/egg/sasl-0.1.1/sasl/__init__.py
R shell/ext-py/egg/sasl-0.1.1/sasl/saslwrapper.cpp
R shell/ext-py/egg/sasl-0.1.1/sasl/saslwrapper.h
R shell/ext-py/egg/sasl-0.1.1/sasl/saslwrapper.py
R shell/ext-py/egg/sasl-0.1.1/sasl/saslwrapper_wrap.cxx
R shell/ext-py/egg/sasl-0.1.1/setup.cfg
R shell/ext-py/egg/sasl-0.1.1/setup.py
D shell/ext-py/sqlparse-0.1.14/AUTHORS
D shell/ext-py/sqlparse-0.1.14/CHANGES
D shell/ext-py/sqlparse-0.1.14/bin/sqlformat
D shell/ext-py/sqlparse-0.1.14/docs/source/changes.rst
D shell/ext-py/sqlparse-0.1.14/pytest.ini
D shell/ext-py/sqlparse-0.1.14/setup.cfg
D shell/ext-py/sqlparse-0.1.14/sqlparse/engine/__init__.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/engine/filter.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/engine/grouping.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/exceptions.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/filters.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/formatter.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/functions.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/lexer.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/pipeline.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/sql.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/tokens.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/utils.py
D shell/ext-py/sqlparse-0.1.14/tests/test_filters.py
D shell/ext-py/sqlparse-0.1.14/tests/test_format.py
D shell/ext-py/sqlparse-0.1.14/tests/test_functions.py
D shell/ext-py/sqlparse-0.1.14/tests/test_grouping.py
D shell/ext-py/sqlparse-0.1.14/tests/test_parse.py
D shell/ext-py/sqlparse-0.1.14/tests/test_pipeline.py
D shell/ext-py/sqlparse-0.1.14/tests/test_regressions.py
D shell/ext-py/sqlparse-0.1.14/tests/test_split.py
D shell/ext-py/sqlparse-0.1.14/tests/test_tokenize.py
D shell/ext-py/sqlparse-0.1.14/tests/utils.py
D shell/ext-py/sqlparse-0.1.14/tox.ini
A shell/ext-py/whl/sqlparse-0.2.4/AUTHORS
A shell/ext-py/whl/sqlparse-0.2.4/CHANGELOG
R shell/ext-py/whl/sqlparse-0.2.4/LICENSE
R shell/ext-py/whl/sqlparse-0.2.4/MANIFEST.in
R shell/ext-py/whl/sqlparse-0.2.4/PKG-INFO
R shell/ext-py/whl/sqlparse-0.2.4/README.rst
R shell/ext-py/whl/sqlparse-0.2.4/TODO
R shell/ext-py/whl/sqlparse-0.2.4/docs/source/analyzing.rst
R shell/ext-py/whl/sqlparse-0.2.4/docs/source/api.rst
A shell/ext-py/whl/sqlparse-0.2.4/docs/source/changes.rst
R shell/ext-py/whl/sqlparse-0.2.4/docs/source/conf.py
R shell/ext-py/whl/sqlparse-0.2.4/docs/source/index.rst
R shell/ext-py/whl/sqlparse-0.2.4/docs/source/indices.rst
R shell/ext-py/whl/sqlparse-0.2.4/docs/source/intro.rst
R shell/ext-py/whl/sqlparse-0.2.4/docs/source/ui.rst
R shell/ext-py/whl/sqlparse-0.2.4/docs/sqlformat.1
A shell/ext-py/whl/sqlparse-0.2.4/setup.cfg
R shell/ext-py/whl/sqlparse-0.2.4/setup.py
R shell/ext-py/whl/sqlparse-0.2.4/sqlparse/__init__.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/__main__.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/cli.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/compat.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/engine/__init__.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/engine/filter_stack.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/engine/grouping.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/engine/statement_splitter.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/exceptions.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/filters/__init__.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/filters/aligned_indent.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/filters/others.py
A 

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8639


Change subject: IMPALA-4664: Unexpected string conversion in Shell
..

IMPALA-4664: Unexpected string conversion in Shell

Impala shell can accidentally convert certain
literal strings to lowercase. Impala shell splits
each command into tokens and then converts the
first token to lowercase to figure out how it
should execute the command. The splitting is done
by spaces only. Thus, if the user types a TAB
after the SELECT, the first token after the split
becomes the SELECT plus whatever comes after it.

Using sqlparse library in infra instead of
shell/ext-py/sqlparse-0.1.14. The sqlparse
library in infra is upgraded from 0.1.15 to
0.2.4 due to bug fixes and syntax extension.

Testing:
Add tests to tests/shell/test_shell_interactive.py

Change-Id: Iba6fa4e9ac570d22135ba51b844db8f9be965ca3
---
M LICENSE.txt
M README.md
M bin/rat_exclude_files.txt
M infra/python/deps/requirements.txt
M shell/.gitignore
D shell/ext-py/sqlparse-0.1.14/AUTHORS
D shell/ext-py/sqlparse-0.1.14/CHANGES
D shell/ext-py/sqlparse-0.1.14/COPYING
D shell/ext-py/sqlparse-0.1.14/MANIFEST.in
D shell/ext-py/sqlparse-0.1.14/PKG-INFO
D shell/ext-py/sqlparse-0.1.14/README.rst
D shell/ext-py/sqlparse-0.1.14/TODO
D shell/ext-py/sqlparse-0.1.14/bin/sqlformat
D shell/ext-py/sqlparse-0.1.14/docs/source/analyzing.rst
D shell/ext-py/sqlparse-0.1.14/docs/source/api.rst
D shell/ext-py/sqlparse-0.1.14/docs/source/changes.rst
D shell/ext-py/sqlparse-0.1.14/docs/source/conf.py
D shell/ext-py/sqlparse-0.1.14/docs/source/index.rst
D shell/ext-py/sqlparse-0.1.14/docs/source/indices.rst
D shell/ext-py/sqlparse-0.1.14/docs/source/intro.rst
D shell/ext-py/sqlparse-0.1.14/docs/source/ui.rst
D shell/ext-py/sqlparse-0.1.14/docs/sqlformat.1
D shell/ext-py/sqlparse-0.1.14/pytest.ini
D shell/ext-py/sqlparse-0.1.14/setup.cfg
D shell/ext-py/sqlparse-0.1.14/setup.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/__init__.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/engine/__init__.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/engine/filter.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/engine/grouping.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/exceptions.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/filters.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/formatter.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/functions.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/keywords.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/lexer.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/pipeline.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/sql.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/tokens.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/utils.py
D shell/ext-py/sqlparse-0.1.14/tests/__init__.py
D shell/ext-py/sqlparse-0.1.14/tests/files/_Make_DirEntry.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/begintag.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/begintag_2.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/dashcomment.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/function.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/function_psql.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/function_psql2.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/function_psql3.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/huge_select.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/test_cp1251.sql
D shell/ext-py/sqlparse-0.1.14/tests/test_filters.py
D shell/ext-py/sqlparse-0.1.14/tests/test_format.py
D shell/ext-py/sqlparse-0.1.14/tests/test_functions.py
D shell/ext-py/sqlparse-0.1.14/tests/test_grouping.py
D shell/ext-py/sqlparse-0.1.14/tests/test_parse.py
D shell/ext-py/sqlparse-0.1.14/tests/test_pipeline.py
D shell/ext-py/sqlparse-0.1.14/tests/test_regressions.py
D shell/ext-py/sqlparse-0.1.14/tests/test_split.py
D shell/ext-py/sqlparse-0.1.14/tests/test_tokenize.py
D shell/ext-py/sqlparse-0.1.14/tests/utils.py
D shell/ext-py/sqlparse-0.1.14/tox.ini
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
63 files changed, 37 insertions(+), 6,715 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/8639/1
--
To view, visit http://gerrit.cloudera.org:8080/8639
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iba6fa4e9ac570d22135ba51b844db8f9be965ca3
Gerrit-Change-Number: 8639
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul