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 <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Kim Jin Chul <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-Comment-Date: Thu, 07 Dec 2017 02:11:56 +0000 Gerrit-HasComments: Yes
