Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ISSUES-5697 fix insert and select query with mysql style identifier #5704

Merged
merged 4 commits into from
Jun 24, 2019

Conversation

zhang2014
Copy link
Contributor

@zhang2014 zhang2014 commented Jun 21, 2019

#5697
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Category:

  • Bug Fix

@blinkov blinkov added can be tested pr-bugfix Pull request with bugfix, not backported by default labels Jun 21, 2019
@@ -255,7 +255,7 @@ inline void writeJSONString(const char * begin, const char * end, WriteBuffer &
}


template <char c>
template <char c, char symbol = '\\'>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The symbol name is unclear.
The fact that we escape all special characters with backslash escept c looks strange. Is it really consistent with MySQL?

Copy link
Contributor Author

@zhang2014 zhang2014 Jun 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MySQL sys.quote_identifier only describes the content of backquotes. When I use it for special character escaping, it has some truncation problems, such as: SELECT sys.quote_identifier('\ b')=`, the following is directly The result of using the AS clause escaping in MySQL, but I'm not sure if this is correct (because I'm not sure if MySQL Client treats them as special characters):

:) SELECT 1 AS `\b`;
+----+
| \b |
+----+
|  1 |
+----+
1 row in set (0.00 sec)

:) SELECT 1 AS `\f`;
+----+
| \f |
+----+
|  1 |
+----+
1 row in set (0.00 sec)

:) SELECT 1 AS `\n`;
+----+
| \n |
+----+
|  1 |
+----+
1 row in set (0.00 sec)

:) SELECT 1 AS `\r`;
+----+
| \r |
+----+
|  1 |
+----+
1 row in set (0.00 sec)

:) SELECT 1 AS `\t`;
+----+
| \t |
+----+
|  1 |
+----+
1 row in set (0.00 sec)

:) SELECT 1 AS `\0`;
+----+
| \0 |
+----+
|  1 |
+----+
1 row in set (0.00 sec)

:) SELECT 1 AS `\\`;
+----+
| \\ |
+----+
|  1 |
+----+
1 row in set (0.00 sec)

Backticks, /// `mysql` style
DoubleQuotes /// "postgres" style
None, /// Write as-is, without quotes.
Backticks, /// `clickhouse` style
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need to support `quoted``identifiers` in MySQL style in ClickHouse parser?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but now ClickHouse uses backslash to escape backquote, and any changes could cause version-compatibility problems. Are we sure we want todo this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot that we already do it:

:) SELECT 1 `a``b`

SELECT 1 AS `a\`b`                                                                                                                                                                                
                                                                                                                                                                                                  
┌─a`b─┐
│   1 │                                                                                                                                                                                           
└─────┘

@@ -255,7 +255,7 @@ inline void writeJSONString(const char * begin, const char * end, WriteBuffer &
}


template <char c>
template <char c, char escape_symbol = '\\'>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have to do something with this name...
because the escape_symbol applies only to c character and C-style escaping is applied to other characters
and it is counterintuitive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

template <
    char quote_character, /// Will escape this character in addition to the list of special characters.
    bool escape_quote_with_quote /// Should we escape quote character with quote character ('hello''world') instead of backslash. Other special characters will be escaped with backslash as usual.
    >

@alexey-milovidov alexey-milovidov merged commit 8e41d89 into ClickHouse:master Jun 24, 2019
alexey-milovidov added a commit that referenced this pull request Jun 24, 2019
stavrolia pushed a commit that referenced this pull request Jul 3, 2019
ISSUES-5697 fix insert and select query with mysql style identifier

(cherry picked from commit 8e41d89)
@stavrolia stavrolia added the v19.9 label Jul 3, 2019
@alesapin alesapin removed the v19.9 label Jul 9, 2019
abyss7 pushed a commit that referenced this pull request Jul 11, 2019
ISSUES-5697 fix insert and select query with mysql style identifier

(cherry picked from commit 8e41d89)
@abyss7 abyss7 added the 19.10 label Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants