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.

Sign up for GitHub

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

Jump to bottom

Coral-Trino: Replace backquotes with equivalent double quotes #243

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+7 −4
Open

Coral-Trino: Replace backquotes with equivalent double quotes #243

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2aa9ef1
Replacing backquotes with equivalent double quotes to fix Trino SqlPa…
KevinGe00 Mar 10, 2022
c107a7f
Union of standardizeSql
KevinGe00 Mar 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions coral-trino/src/main/java/com/linkedin/coral/trino/trino2rel/TrinoToRelConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ protected SqlToRelConverter getSqlToRelConverter() {

@Override
protected SqlNode toSqlNode(String sql, Table trinoView) {
String trimmedSql = trimParenthesis(sql.toUpperCase());
String trimmedSql = standardizeSql(sql);
SqlNode parsedSqlNode = PrestoParserDriver.parse(trimmedSql).accept(parseTreeBuilder, parserVisitorContext);
SqlNode convertedSqlNode = parsedSqlNode.accept(new Trino2CoralOperatorConverter());
return convertedSqlNode;
Expand All @@ -94,12 +94,15 @@ protected RelNode standardizeRel(RelNode relNode) {
return relNode;
}

private static String trimParenthesis(String value) {
/*
* Standardizing Sql to ensure compatability with Sql Parser.
*/
private static String standardizeSql(String value) {
String str = value.trim();
if (str.startsWith("(") && str.endsWith(")")) {
return trimParenthesis(str.substring(1, str.length() - 1));
return standardizeSql(str.substring(1, str.length() - 1));
}
return str;
return str.toUpperCase().replaceAll("`", "\"");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe putting replaceAll here should avoid unnecessary calls if we need to recurse, would appreciate confirmation on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The toUpperCase() is brought over from this line.

}

}