Skip to content

Revert "Preserve quotedness for Identifier when formatting Query"#17446

Merged
rschlussel merged 1 commit into
prestodb:masterfrom
varungajjala:revertquotedness
Mar 9, 2022
Merged

Revert "Preserve quotedness for Identifier when formatting Query"#17446
rschlussel merged 1 commit into
prestodb:masterfrom
varungajjala:revertquotedness

Conversation

@varungajjala

@varungajjala varungajjala commented Mar 9, 2022

Copy link
Copy Markdown
Contributor

This reverts commit d8323f5.

== NO RELEASE NOTE ==

@varungajjala

varungajjala commented Mar 9, 2022

Copy link
Copy Markdown
Contributor Author

cc @v-jizhang

This is failing some of the tests. Please look at the stack trace below.

java.lang.AssertionError: expected [CREATE VIEW hive_view.hive_test.hive AS
SELECT *
FROM
  orders] but found [CREATE VIEW hive.hive_test.hive_view AS
SELECT *
FROM
  orders]
	at org.testng.Assert.fail(Assert.java:94)
	at org.testng.Assert.failNotEquals(Assert.java:513)
	at org.testng.Assert.assertEqualsImpl(Assert.java:135)
	at org.testng.Assert.assertEquals(Assert.java:116)
	at org.testng.Assert.assertEquals(Assert.java:179)
	at com.facebook.presto.spark.TestPrestoSparkQueryRunner.testCreateDropView(TestPrestoSparkQueryRunner.java:993)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:645)
	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

@aweisberg aweisberg requested a review from v-jizhang March 9, 2022 18:07
aweisberg
aweisberg approved these changes Mar 9, 2022
View reviewed changes

@aweisberg aweisberg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Assuming the tests pass +1

highker
highker approved these changes Mar 9, 2022
View reviewed changes

@highker highker left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

would be good to have @rschlussel to take a look as well

rschlussel
rschlussel approved these changes Mar 9, 2022
View reviewed changes

@rschlussel rschlussel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This issue isn't just Presto on Spark, so we're lucky presto on spark had that round trip test. We should add better basic tests for show Create View and show Create Table to regular Presto.

Comment thread presto-main/src/main/java/com/facebook/presto/sql/rewrite/ShowQueriesRewrite.java
ordering(ascending("argument_types")));
}

private QualifiedName getQualifiedName(ShowCreate node, QualifiedObjectName objectName)

@rschlussel rschlussel Mar 9, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@v-jizhang looks like this is getting the parts for the name backwards.

@v-jizhang v-jizhang Mar 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Trino still has the same code
https://github.com/trinodb/trino/blob/99ef4ec162b3b099762e4d6727ee3db641e1445c/core/trino-main/src/main/java/io/trino/sql/rewrite/ShowQueriesRewrite.java#L593

This PR was requested by some users, I'll do a fix and resubmit.

@rschlussel rschlussel merged commit eff7788 into prestodb:master Mar 9, 2022
@rschlussel rschlussel mentioned this pull request Jul 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@highker highker highker approved these changes

@rschlussel rschlussel rschlussel approved these changes

@v-jizhang v-jizhang Awaiting requested review from v-jizhang

@tdcmeehan tdcmeehan Awaiting requested review from tdcmeehan

+1 more reviewer

@aweisberg aweisberg aweisberg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@varungajjala @aweisberg @highker @rschlussel @v-jizhang