Coral-Trino: Replace backquotes with equivalent double quotes #243
Conversation
…rser exitBackQuotedIdentifier error
| private static String standardizeSql(String sql) { | ||
| return sql.toUpperCase().replaceAll("`", "\""); | ||
| } | ||
|
|
||
| private static String trimParenthesis(String value) { | ||
| String str = value.trim(); | ||
| if (str.startsWith("(") && str.endsWith(")")) { |
There was a problem hiding this comment.
I think it's better to combine these 2 methods together in one method standardizeTrinoSql and add comment of the reason why we need to standardize it.
ljfgem
changed the title
| } | ||
| return str; | ||
| return str.toUpperCase().replaceAll("`", "\""); |
There was a problem hiding this comment.
I believe putting replaceAll here should avoid unnecessary calls if we need to recurse, would appreciate confirmation on that.
There was a problem hiding this comment.
Why did we add toUppercase(). Also back quotes can be part of the literals so might accidentally change a backquote that was not supposed to be changed.
There was a problem hiding this comment.
The toUpperCase() is brought over from this line.
wmoustafa
changed the title
|
I am not clear on the motivation. So obviously, current test cases do not have this issue. Did you run to them in production views (that they use backquotes)? If yes, how did the production views get created in the first place? You might want to add more details to the description to make the workflow of the issue clear. |
|
By any chance are we using the wrong dialect to generate this SQL? Dialects allow customizing the quotes if that was the issue. See https://github.com/linkedin/coral/blob/master/coral-trino/src/main/java/com/linkedin/coral/trino/rel2trino/TrinoSqlDialect.java#L18. |
Fixes issue #242.