Preserve quotedness in QualifiedName by imjalpreet · Pull Request #19827 · prestodb/presto

imjalpreet · 2023-06-08T15:05:04Z

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

Test plan

Before this PR, presto throws an error when using a reserved keyword in a prepared statement or while creating a view.

presto:imjalpreet> create view group_view security invoker as select * from imjalpreet."group";

Query 20230530_141710_00001_fxwah failed: Formatted query does not parse: Query{queryBody=QuerySpecification{select=Select{distinct=false, selectItems=[*]}, from=Optional[Table{imjalpreet.group}], where=null, groupBy=Optional.empty, having=null, orderBy=Optional.empty, offset=null, limit=null}, orderBy=Optional.empty}

presto:imjalpreet> PREPARE stmt0 FROM select * from imjalpreet."group";

Query 20230530_142117_00003_fxwah failed: Formatted query does not parse: Query{queryBody=QuerySpecification{select=Select{distinct=false, selectItems=[*]}, from=Optional[Table{imjalpreet.group}], where=null, groupBy=Optional.empty, having=null, orderBy=Optional.empty, offset=null, limit=null}, orderBy=Optional.empty}

After this PR, queries runs successfully.

presto:imjalpreet> create view group_view security invoker as select * from imjalpreet."group";
CREATE VIEW

presto:imjalpreet> PREPARE stmt0 FROM select * from imjalpreet."group";
PREPARE

== RELEASE NOTES ==

General Changes
* Preserve table name quoting in the output of ``SHOW CREATE VIEW``.
* Fix failure when preparing statements or creating views that contain a quoted reserved word as a table name.

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 <praveenkrishna@tutanota.com>

linux-foundation-easycla · 2023-06-08T15:05:10Z

✅ login: imjalpreet / name: Jalpreet Singh Nanda (e49964c)
❌ - login: @Praveen2112 / name: praveenkrishna . The commit (e49964c) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

imjalpreet · 2023-06-08T15:06:49Z

@agrawalreetika Added the tests as mentioned in #19752

imjalpreet · 2023-06-29T15:04:49Z

@prestodb/committers Hi, can someone help with the review for this PR?

rschlussel

Last time we had this change, it needed to be reverted do to a bug in creating the qualified name for views. Can you check that the issue here is resolved and add a test #17446

rschlussel · 2023-07-13T14:43:56Z

+        private QualifiedName getQualifiedName(ShowCreate node, QualifiedObjectName objectName)
+        {
+            List<Identifier> parts = node.getName().getOriginalParts();
+            Identifier tableName = (parts.size() > 2) ? parts.get(2) : ((parts.size() > 1) ? parts.get(1) : parts.get(0));


can simplify to Identifier tableName = parts.get(parts.size() -1)

rschlussel · 2023-07-13T14:45:45Z

+        {
+            List<Identifier> parts = node.getName().getOriginalParts();
+            Identifier tableName = (parts.size() > 2) ? parts.get(2) : ((parts.size() > 1) ? parts.get(1) : parts.get(0));
+            Identifier schemaName = (parts.size() > 2) ? parts.get(1) : ((parts.size() > 1) ? parts.get(0) : new Identifier(objectName.getSchemaName()));


Identifier schemaName = parts.size() > 1 ? : parts.get(parts.size() - 2) : new Identifier(objectName.getSchemaName())

imjalpreet · 2023-07-13T15:32:53Z

@rschlussel I just had a look at the PR you mentioned above and I have already fixed the issue seen due to which it was reverted.

I have already added some tests to test this PR in AbstractTestDistributedQueries#testViewWithReservedKeywords. Let me know if you meant writing a test for generating qualified name for a view.

rschlussel · 2023-07-13T15:38:31Z

@rschlussel I just had a look at the PR you mentioned above and I have already fixed the issue seen due to which it was reverted.

I have already added some tests to test this PR in AbstractTestDistributedQueries#testViewWithReservedKeywords. Let me know if you meant writing a test for generating qualified name for a view.

Thanks, I see now you've fixed it. I think we need the test to also run SHOW CREATE VIEW on the view that it created to exercise the codepath you fixed.

rschlussel · 2023-07-13T15:43:03Z

+        assetQuery("SELECT id\nFROM\n  \"public\".\"order\"\"2\"\n");
+    }
+
+    private void assetQuery(String query)


typo: asset -> assert, but maybe give this function a more descriptive name so you don't have to read the code for this function to understand what the test is doing, e.g. assertFormattedEqualsOriginal

rschlussel · 2023-07-13T15:58:07Z

@@ -76,7 +74,20 @@ public Identifier getField()
     */
    public static QualifiedName getQualifiedName(DereferenceExpression expression)


I know you aren't changing the signature here, but in Presto we usually avoid returning nulls anywhere, and prefer explicitly using optionals if something might not be there. Looks like there are a bunch of usages of this function that aren't checking for null return values. Would be nice as a separate commit to switch to using optional instead of returning null to be sure this is used correctly throughout.

rschlussel · 2023-07-13T16:00:51Z

+    {
+        assetQuery("SELECT id\nFROM\n  public.orders\n");
+        assetQuery("SELECT id\nFROM\n  \"public\".\"order\"\n");
+        assetQuery("SELECT id\nFROM\n  \"public\".\"order\"\"2\"\n");


what kind of format is this last test. looks like

SELECT id FROM "public"."order""2"?

Can you add a test for a three part schema?
Also can you add a test where only one or two parts of the qualified name are quoted

agrawalreetika · 2025-10-28T07:21:01Z

This is already covered as part of #25528

imjalpreet requested a review from a team as a code owner June 8, 2023 15:05

imjalpreet self-assigned this Jun 8, 2023

imjalpreet requested a review from presto-oss June 8, 2023 15:05

imjalpreet requested a review from agrawalreetika June 8, 2023 15:07

rschlussel reviewed Jul 13, 2023

View reviewed changes

wanglinsong requested review from feilong-liu and jaystarshot as code owners July 6, 2024 04:32

imjalpreet mentioned this pull request Jul 17, 2024

PREPARE fails for INSERT statements using non-standard characters (ex: '-') in quoted identifiers #23187

Open

agrawalreetika mentioned this pull request Jul 14, 2025

Preserve quotedness in QualifiedName #25528

Merged

agrawalreetika closed this Oct 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve quotedness in QualifiedName#19827

Preserve quotedness in QualifiedName#19827
imjalpreet wants to merge 1 commit into
prestodb:masterfrom
imjalpreet:preserveQuotedness

imjalpreet commented Jun 8, 2023

Uh oh!

linux-foundation-easycla Bot commented Jun 8, 2023

Uh oh!

imjalpreet commented Jun 8, 2023

Uh oh!

imjalpreet commented Jun 29, 2023

Uh oh!

rschlussel left a comment

Uh oh!

rschlussel Jul 13, 2023

Uh oh!

rschlussel Jul 13, 2023

Uh oh!

imjalpreet commented Jul 13, 2023

Uh oh!

rschlussel commented Jul 13, 2023

Uh oh!

rschlussel Jul 13, 2023

Uh oh!

rschlussel Jul 13, 2023

Uh oh!

rschlussel Jul 13, 2023

Uh oh!

agrawalreetika commented Oct 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -76,7 +74,20 @@ public Identifier getField()
		*/
		public static QualifiedName getQualifiedName(DereferenceExpression expression)

Conversation

imjalpreet commented Jun 8, 2023

Uh oh!

linux-foundation-easycla Bot commented Jun 8, 2023

Uh oh!

imjalpreet commented Jun 8, 2023

Uh oh!

imjalpreet commented Jun 29, 2023

Uh oh!

rschlussel left a comment

Choose a reason for hiding this comment

Uh oh!

rschlussel Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

rschlussel Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

imjalpreet commented Jul 13, 2023

Uh oh!

rschlussel commented Jul 13, 2023

Uh oh!

rschlussel Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

rschlussel Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

rschlussel Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

agrawalreetika commented Oct 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants