You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Kim Jin Chul (Code Review)" <ge...@cloudera.org> on 2017/11/23 16:36:34 UTC
[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell
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 <ji...@gmail.com>
[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
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 agree it would be nice to have a minimal fix for the bug but I think usin
Yes, here is the fix for explicit lower-casting issue:
https://gerrit.cloudera.org/#/c/8762/
What do you think about if the change(i.e. token split issue) will be a next of the change(i.e. lower casting)?
--
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 <ji...@gmail.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Dec 2017 23:46:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
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:
> 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.
I would like to clarify the above comment. I meant syntactical analyzer(not semantic analyzer). Here is additional explanation why the customization is required. For example, Impala 3.0 introduce a new keyword, but sqlparse does not support the keyword officially. We may need to manage the keywords with Impala's version. Please see the case of postgreSQL: https://github.com/andialbrecht/sqlparse/blob/master/sqlparse/keywords.py#L805
--
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 <ji...@gmail.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Dec 2017 04:58:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
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 <ji...@gmail.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Dec 2017 02:11:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
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 <ji...@gmail.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8639/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8639/2//COMMIT_MSG@18
PS2, Line 18: Using sqlparse library in infra instead of
It looks like this has a side-effect of not packaging sql-parse with the shell tarball. Unfortunately we don't have tests for the shell tarball artifact, but I think this will break it. See make_shell_tarball.sh
--
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: 2
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Nov 2017 18:12:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <ji...@gmail.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Dec 2017 23:32:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Kim Jin Chul has uploaded a new patch set (#2). ( 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, 36 insertions(+), 6,715 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/8639/2
--
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: newpatchset
Gerrit-Change-Id: Iba6fa4e9ac570d22135ba51b844db8f9be965ca3
Gerrit-Change-Number: 8639
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
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 (#3).
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 shell/ext-py/whl/sqlparse-0.2.4/sqlparse/filters/output.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/filters/reindent.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/filters/right_margin.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/filters/tokens.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/formatter.py
R shell/ext-py/whl/sqlparse-0.2.4/sqlparse/keywords.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/lexer.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/sql.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/tokens.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/utils.py
R shell/ext-py/whl/sqlparse-0.2.4/tests/__init__.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/conftest.py
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/_Make_DirEntry.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/begintag.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/begintag_2.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/dashcomment.sql
A shell/ext-py/whl/sqlparse-0.2.4/tests/files/encoding_gbk.sql
A shell/ext-py/whl/sqlparse-0.2.4/tests/files/encoding_utf8.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/function.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/function_psql.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/function_psql2.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/function_psql3.sql
A shell/ext-py/whl/sqlparse-0.2.4/tests/files/function_psql4.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/huge_select.sql
A shell/ext-py/whl/sqlparse-0.2.4/tests/files/stream.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/test_cp1251.sql
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_cli.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_format.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_grouping.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_keywords.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_parse.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_regressions.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_split.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_tokenize.py
A shell/ext-py/whl/sqlparse-0.2.4/tox.ini
M shell/impala-shell
M shell/impala_shell.py
M shell/make_shell_tarball.sh
M tests/shell/test_shell_interactive.py
120 files changed, 6,128 insertions(+), 5,137 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/8639/3
--
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: newpatchset
Gerrit-Change-Id: Iba6fa4e9ac570d22135ba51b844db8f9be965ca3
Gerrit-Change-Number: 8639
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
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 shell/ext-py/whl/sqlparse-0.2.4/sqlparse/filters/output.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/filters/reindent.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/filters/right_margin.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/filters/tokens.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/formatter.py
R shell/ext-py/whl/sqlparse-0.2.4/sqlparse/keywords.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/lexer.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/sql.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/tokens.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/utils.py
R shell/ext-py/whl/sqlparse-0.2.4/tests/__init__.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/conftest.py
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/_Make_DirEntry.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/begintag.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/begintag_2.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/dashcomment.sql
A shell/ext-py/whl/sqlparse-0.2.4/tests/files/encoding_gbk.sql
A shell/ext-py/whl/sqlparse-0.2.4/tests/files/encoding_utf8.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/function.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/function_psql.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/function_psql2.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/function_psql3.sql
A shell/ext-py/whl/sqlparse-0.2.4/tests/files/function_psql4.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/huge_select.sql
A shell/ext-py/whl/sqlparse-0.2.4/tests/files/stream.sql
R shell/ext-py/whl/sqlparse-0.2.4/tests/files/test_cp1251.sql
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_cli.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_format.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_grouping.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_keywords.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_parse.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_regressions.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_split.py
A shell/ext-py/whl/sqlparse-0.2.4/tests/test_tokenize.py
A shell/ext-py/whl/sqlparse-0.2.4/tox.ini
M shell/impala-shell
M shell/impala_shell.py
M shell/make_shell_tarball.sh
M tests/shell/test_shell_interactive.py
120 files changed, 6,128 insertions(+), 5,137 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/8639/4
--
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: newpatchset
Gerrit-Change-Id: Iba6fa4e9ac570d22135ba51b844db8f9be965ca3
Gerrit-Change-Number: 8639
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
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 1:
shell/ext-py/sqlparse-0.1.14 includes only one fix for escaping backslashes. The fix was delivered to upstream by this change: https://github.com/andialbrecht/sqlparse/commit/e75e35869473832a1eb67772b1adfee2db11b85a
Therefore, we don't have to keep this library on our repository. The version 0.1.14 is too old which was released in Nov 30, 2014. I would recommend to use recent and stable version 0.2.4. The old version has bugs for parsing such as following cases:
* Variable assignment. The problem is that the tokens '=' '5' disappear.
(e.g. set mt_dop=5;)
* A series of queires in a single row.
(e.g. select'DIG';select'DIG';)
Please see the changelog of sqlparse: https://github.com/andialbrecht/sqlparse/blob/master/CHANGELOG
--
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: 1
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Comment-Date: Thu, 23 Nov 2017 16:48:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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)
I had a suggestion for changing the approach after I spent some time understanding the change. Let me know what you think.
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.
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().
--
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 <ji...@gmail.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Dec 2017 00:42:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
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 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8639/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8639/2//COMMIT_MSG@18
PS2, Line 18: Using sqlparse library in infra instead of
> It looks like this has a side-effect of not packaging sql-parse with the sh
Thanks for the information. The patch set#4 supports wheel packaging for sqlparse-0.2.4. I confirmed locally that the shell tarball should recognize the wheel package and execute impala-shell successfully.
--
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: 2
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Nov 2017 05:07:33 +0000
Gerrit-HasComments: Yes