-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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.
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
Make sure that quoted identifiers remain quoted in formatter #11171
Make sure that quoted identifiers remain quoted in formatter #11171
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SqlFormatter
has a dedicated method for formatting QualifiedName
:
static String formatName(QualifiedName name) |
SetSession
's name (and some other QualifiedNames
s, e.g. in ResetSession
). That should fix the issue.
Please note that neither of the formatting solutions respects the proper identifier semantics. However, the method I suggest is closer and easier to fix, since it delegates the formatting to Identifier
.
01704eb
to
25f136b
Compare
Thank you, @kasiafi , the |
78b793f
to
c6e3fb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a side-effect, this change alters the output of SqlFormatter regarding QualifiedName
s in SetSession
and ResetSession
: before they were lowercased; after -- they are not.
It has no practical meaning, because the affected QualifiedName
s are compared and resolved case-insensitive. However, please add this info to the commit message.
Many other Nodes have fields of type QualifiedName
, too:
AddColumn
Analyze
Call
Comment
CreateMaterializedView
CreateSchema
CreateTable
CreateTableAsSelect
CreateView
Deny
DropColumn
DropMaterializedView
DropSchema
DropTable
DropView
FunctionCall
Grant
LikeClause
RenameColumn
RenameMaterializedView
RenameSchema
RenameTable
RenameView
ResetSession
Revoke
SetAuthorizationStatement
SetProperties
SetSession
ShowColumns
ShowCreate
Table
TruncateTable
Some of those are formatted with the formatName
method, and some use toString
. Could you please file an issue to check and fix the other occurrences in SqlFormatter
and ExpressionFormatter
?
ce7ad9d
to
648a784
Compare
The side-effect of this change is that formatted statements for SetSession and ResetSession are no longer lowercased as before. However QualifiedName instances are resolved and compared case-insensitive, thus such side-effect does not lead to any consequences.
648a784
to
fa8f2c3
Compare
CI hit failure uploading test report to GitHub. nothing actually failed. |
Cherry-pick of trinodb/trino#11171 The SqlFormatter effectively drops quotes from the quoted identifiers when formatting 'SET SESSION' statement. This causes valid queries to fail: e.g. SET SESSION "name-suffix".property = 'value' PrestoException: Formatted query does not parse even when identifier (name-suffix) is quoted. Follow up prestodb#17186 Co-authored-by: Sergey Melnychuk <[email protected]>
The
SqlFormatter
effectively drops quotes from the quoted identifiers when formatting 'SET SESSION' statement (https://github.com/trinodb/trino/blob/master/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java#L1514).This causes valid queries to fail: e.g.
SET SESSION "name-suffix".property = 'value'
throwsio.trino.spi.TrinoException: Formatted query does not parse
even when identifier (name-suffix
) is quoted.