Preserve quotedness for Identifier when formatting Query #80

Praveen2112 · 2019-01-26T17:18:01Z

Presto doesn't maintain the quotedness of an identifier when using SqlQueryFormatter so it results in throwing parsing error when we run prepare query of the syntax [https://github.com/prestodb/presto/issues/10739]. This PR solves that above issue

martint · 2019-01-29T23:59:21Z

presto-parser/src/main/java/io/prestosql/sql/tree/DereferenceExpression.java

@@ -76,7 +76,7 @@ public Identifier getField()
     */
    public static QualifiedName getQualifiedName(DereferenceExpression expression)
    {
-        List<String> parts = tryParseParts(expression.base, expression.field.getValue().toLowerCase(Locale.ENGLISH));
+        List<Identifier> parts = tryParseParts(expression.base, expression.field.getValue().toLowerCase(Locale.ENGLISH));


It shouldn't be necessary to call toLowerCase(). The DereferenceExpression already deals with Identifiers

In fact, I don't think the tryParseParts method needs to exist anymore in its current form.

martint · 2019-01-30T00:23:18Z

presto-parser/src/test/java/io/prestosql/sql/parser/TestSqlParser.java

@@ -1844,7 +1845,7 @@ public void testShowStats()
        final String[] tableNames = {"t", "s.t", "c.s.t"};

        for (String fullName : tableNames) {
-            QualifiedName qualifiedName = QualifiedName.of(Arrays.asList(fullName.split("\\.")));
+            QualifiedName qualifiedName = QualifiedName.of(Arrays.asList(fullName.split("\\.")).stream().map(Identifier::new).collect(Collectors.toList()));


This is a little hard to read. I'd pull it out into a makeQualifiedName. Also, I'd structure it as:

List<Identifier> parts = Arrays.stream(fullName.split("\\.")) .map(Identifier::new) .collect(Collectors.toList()); return QualifiedName.of(parts);

martint · 2019-01-30T00:27:31Z

presto-parser/src/test/java/io/prestosql/sql/parser/TestSqlParser.java

@@ -1855,7 +1856,7 @@ public void testShowStatsForQuery()
        final String[] tableNames = {"t", "s.t", "c.s.t"};

        for (String fullName : tableNames) {
-            QualifiedName qualifiedName = QualifiedName.of(Arrays.asList(fullName.split("\\.")));
+            QualifiedName qualifiedName = QualifiedName.of(Arrays.asList(fullName.split("\\.")).stream().map(Identifier::new).collect(Collectors.toList()));


martint · 2019-01-30T00:32:02Z

presto-verifier/src/main/java/io/prestosql/verifier/VerifierConfig.java

@@ -734,7 +736,7 @@ public VerifierConfig setShadowWrites(boolean shadowWrites)

    public QualifiedName getShadowTestTablePrefix()
    {
-        return QualifiedName.of(Splitter.on(".").splitToList(shadowTestTablePrefix));
+        return QualifiedName.of(Splitter.on(".").splitToList(shadowTestTablePrefix).stream().map(Identifier::new).collect(Collectors.toList()));


This can be changed to Arrays.stream(fullName.split("\\.")).xxx like the calls in TestSqlParser and formatted across multiple lines to make it more readable.

martint · 2019-01-30T00:32:08Z

presto-verifier/src/main/java/io/prestosql/verifier/VerifierConfig.java

@@ -747,7 +749,7 @@ public VerifierConfig setShadowTestTablePrefix(String prefix)

    public QualifiedName getShadowControlTablePrefix()
    {
-        return QualifiedName.of(Splitter.on(".").splitToList(shadowControlTablePrefix));
+        return QualifiedName.of(Splitter.on(".").splitToList(shadowControlTablePrefix).stream().map(Identifier::new).collect(Collectors.toList()));


martint · 2019-01-30T00:34:12Z

presto-main/src/test/java/io/prestosql/sql/TestSqlFormatterUtil.java

+import static io.prestosql.sql.SqlFormatterUtil.getFormattedSql;
+import static org.testng.Assert.assertEquals;
+
+public class TestSqlFormatterUtil


Call this TestSqlFormatter

martint · 2019-01-30T00:44:43Z

presto-main/src/main/java/io/prestosql/sql/rewrite/ShowQueriesRewrite.java

@@ -373,7 +374,7 @@ protected Node visitShowCreate(ShowCreate node, Void context)
                }

                Query query = parseView(viewDefinition.get().getOriginalSql(), objectName, node);
-                String sql = formatSql(new CreateView(createQualifiedName(objectName), query, false, Optional.empty()), Optional.of(parameters)).trim();
+                String sql = formatSql(new CreateView(createQualifiedName(objectName, MetadataUtil.getDelimitedList(node.getName())), query, false, Optional.empty()), Optional.of(parameters)).trim();


This seems unnecessary. It looks like you could just do: new CreateView(node.getName(), ...) instead of trying to recreate the QualifiedName from the QualifiedObjectName that was derived from node.getName().

If you do that, you should also be able to remove MetadataUtil.getDelimitedList(), which I'm not fond of.

martint

Looks good. Can you rebase and fix the merge conflicts? I'll merge it once that's taken care of.

Praveen2112 · 2019-02-02T18:03:36Z

@martint I have updated the code and resolved the conflicts.

martint · 2019-02-03T04:11:59Z

@Praveen2112, there are some build failures. Can you take a look?

We maintain Identifier in QualifiedName for preserving its quotedness.

Cherry-pick of trinodb/trino#80 Presto doesn't maintain the quotedness of an identifier when using SqlQueryFormatter so it results in throwing parsing error when we run prepare query of the syntax [prestodb#10739]. This PR solves that above issue. Co-authored-by: praveenkrishna <[email protected]>

Cherry-pick of trinodb/trino#80 Presto doesn't maintain the quotedness of an identifier when using SqlQueryFormatter so it results in throwing parsing error when we run prepare query of the syntax [#10739]. This PR solves that above issue. Co-authored-by: praveenkrishna <[email protected]>

Cherry-pick of trinodb/trino#80 Presto doesn't maintain the quotedness of an identifier when using SqlQueryFormatter so it results in throwing parsing error if quoted table name is a reserved word Co-authored-by: praveenkrishna <[email protected]>

cla-bot bot added the cla-signed label Jan 26, 2019

martint self-assigned this Jan 28, 2019

martint requested changes Jan 30, 2019

View reviewed changes

martint assigned Praveen2112 and unassigned martint Jan 30, 2019

Praveen2112 force-pushed the maintain_quotedness branch from 5eb61f5 to 0ca9718 Compare January 31, 2019 10:14

martint approved these changes Feb 1, 2019

View reviewed changes

Praveen2112 force-pushed the maintain_quotedness branch from 0ca9718 to 14558fb Compare February 2, 2019 17:55

Praveen2112 force-pushed the maintain_quotedness branch from 14558fb to be38a23 Compare February 3, 2019 13:19

Preserve quotedness in QualifiedName

70e6f1b

We maintain Identifier in QualifiedName for preserving its quotedness.

Praveen2112 force-pushed the maintain_quotedness branch from be38a23 to 70e6f1b Compare February 4, 2019 17:01

martint merged commit d5dfc99 into trinodb:master Feb 4, 2019

martint mentioned this pull request Feb 4, 2019

Release notes for 302 #125

Closed

6 tasks

v-jizhang mentioned this pull request Jan 13, 2022

Preserve quotedness for Identifier when formatting Query prestodb/presto#17186

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve quotedness for Identifier when formatting Query #80

Preserve quotedness for Identifier when formatting Query #80

Praveen2112 commented Jan 26, 2019

martint Jan 29, 2019

martint Jan 30, 2019

martint Jan 30, 2019

martint Jan 30, 2019

martint Jan 30, 2019

martint Jan 30, 2019

martint Jan 30, 2019

martint Jan 30, 2019

martint left a comment

Praveen2112 commented Feb 2, 2019

martint commented Feb 3, 2019

Preserve quotedness for Identifier when formatting Query #80

Preserve quotedness for Identifier when formatting Query #80

Conversation

Praveen2112 commented Jan 26, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martint left a comment

Choose a reason for hiding this comment

Praveen2112 commented Feb 2, 2019

martint commented Feb 3, 2019