ADD parsing option for column names containing spaces #35

mknorps · 2018-11-09T10:13:53Z

Hi,
I encountered a problem when parsing SQL queries with column names that contained special characters like space. In MySql (I use mariaDB) such columns are put into backticks, so a valid query yields:

SELECT `user ID`
FROM a

I added a test and parsing option to the sql_parse module.

Please merge it if you find it useful.

klahnakoski · 2018-11-09T15:05:49Z

requirements.txt

@@ -1,2 +1,3 @@
 mo-future
 pyparsing
+six


I did not see where six is being used. Did I miss something?

it is used in formatting.py, line 16

oh! good catch! Please remove the six from requirements.txt and add it to tests/requirements.txt

klahnakoski · 2018-11-09T15:14:45Z

Please add a test to https://github.com/mozilla/moz-sql-parser/blob/dev/tests/test_simple.py that confirms the backtick string is parsed properly. I will look into you other test to see why it appears to misbehave.

Thank you!

klahnakoski · 2018-11-09T15:08:16Z

tests/test_format_and_parse.py

+
+    def test_191(self):
+        expected_sql = "SELECT `user ID` FROM a"
+        expected_json = {'select': {'value': '`user ID`'}, 'from': 'a'}


We should not see the backticks in the expected json. Maybe there is a bigger problem in verify_formatting that is letting this test pass

Should I keep this test?
The other option would be, as you mentioned, modify escape function to use non-ansi quotes (`) for specific column names.

This test must change. Change expected_json to not include the backticks:

expected_json = {'select': {'value': 'user ID'}, 'from': 'a'}

Actually, forget my last message, and remove this test. The formatting.py generates double quoted column names

I marked test as skipped, so limits of formatting.py are known.

klahnakoski · 2018-11-09T15:32:08Z

OK, I think I see part of the problem:

moz-sql-parser/moz_sql_parser/formatting.py

Line 40 in b30c7c9

def escape(identifier, ansi_quotes, should_quote):

The escape function does not recognize the backticks, and is wrapping the identifier in double quotes. the next question is why are the backticks still there.

klahnakoski · 2018-11-09T15:44:47Z

I made this test (you can add it on your end, and push them to your branch so it becomes part of this PR)

def test_backtick(self):
    result = parse("SELECT `user ID` FROM a")
    expected = {'select': {'value': 'user ID'}, 'from': 'a'}
    self.assertEqual(result, expected)

which I see a parsing problem at

https://github.com/mozilla/moz-sql-parser/blob/b30c7c9832dc605d8951646c3c53bf0bb778e7ab/moz_sql_parser/sql_parser.py#L225..L226

Please verify how a backtick in such a string is escaped; does it use backslash (\` ) or double backtick (``)?

ADD simple backticks test

klahnakoski · 2018-11-13T13:15:05Z

tests/test_simple.py

+        expected = {'select': {'value': 'user ID'}, 'from': 'a'}
+        self.assertEqual(result, expected)
+
+    def test_backticki_escape(self):


excellent! I hoped you would add his type of test.

klahnakoski · 2018-11-13T13:27:05Z

moz_sql_parser/sql_parser.py

@@ -222,6 +222,8 @@ def unquote(instring, tokensStart, retTokens):
    elif val.startswith('"') and val.endswith('"'):
        val = '"'+val[1:-1].replace('""', '\\"')+'"'
        # val = val.replace(".", "\\.")
+    elif val.startswith('`') and val.endswith('`'):
+          val = "'" + val[1:-1].replace("''", "\\'").replace("``","`") + "'"


please remove the .replace("''", "\\'") from this line. As far as I can tell, sequential single quotes ('') are not converted to a single quote inside a backtick string.

ok, done in current commit

ADD handling for 'LEFT JOIN' in parser ADD test for 'LEFT JOIN' handling SKIP test in format_and_parse for backticks and 'LEFT JOIN'

mknorps · 2018-11-13T20:22:12Z

Using the package longer I found one more missing element - LEFT JOIN.

The addition is so small, I pushed it in the same PR.
Tests are updated.

Than you very much for creating this package. It is very useful and compact.

klahnakoski · 2018-11-13T21:39:09Z

tests/test_format_and_parse.py

@@ -1080,3 +1080,19 @@ def test_190(self):
        expected_sql = "SELECT typeof(sum(a3)) FROM a GROUP BY a1"
        expected_json = {'from': 'a', 'select': {'value': {'typeof': {'sum': 'a3'}}}, 'groupby': {'value': 'a1'}}
        self.verify_formatting(expected_sql, expected_json)
+
+    @skip("The escape function does not recognize the backticks,"


Good idea to skip the tests! If someone works on a MySQL formatter, they have something to start with.

ADD parsing option for column names containing spaces

b30c7c9

klahnakoski reviewed Nov 9, 2018

View reviewed changes

mknorps added 2 commits November 11, 2018 10:47

FIX requirements

35a63d3

ADD simple backticks test

ADD backtick escape

1631b84

klahnakoski reviewed Nov 13, 2018

View reviewed changes

DEL replace("''", "\\'") for backticks

8b85245

ADD handling for 'LEFT JOIN' in parser ADD test for 'LEFT JOIN' handling SKIP test in format_and_parse for backticks and 'LEFT JOIN'

klahnakoski merged commit b39fbee into mozilla:dev Nov 13, 2018

klahnakoski reviewed Nov 13, 2018

View reviewed changes

enilsen16 mentioned this pull request Aug 13, 2019

Backticks in table name are being incorrectly escaped #92

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADD parsing option for column names containing spaces #35

ADD parsing option for column names containing spaces #35

mknorps commented Nov 9, 2018

klahnakoski Nov 9, 2018

mknorps Nov 9, 2018

klahnakoski Nov 9, 2018

klahnakoski commented Nov 9, 2018

klahnakoski Nov 9, 2018

mknorps Nov 11, 2018

klahnakoski Nov 13, 2018

klahnakoski Nov 13, 2018

mknorps Nov 13, 2018

klahnakoski commented Nov 9, 2018

klahnakoski commented Nov 9, 2018

klahnakoski Nov 13, 2018

klahnakoski Nov 13, 2018

mknorps Nov 13, 2018

mknorps commented Nov 13, 2018

klahnakoski Nov 13, 2018

ADD parsing option for column names containing spaces #35

ADD parsing option for column names containing spaces #35

Conversation

mknorps commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klahnakoski commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klahnakoski commented Nov 9, 2018

klahnakoski commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mknorps commented Nov 13, 2018

Choose a reason for hiding this comment