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.

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

Feat: add the ability to set meta in sql comments #2351

Merged
merged 1 commit into from
Oct 2, 2023
Merged

Conversation

tobymao
Copy link
Owner

@tobymao tobymao commented Sep 30, 2023

No description provided.

Copy link
Collaborator

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

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

I can see how this adds value, though I am a little worried about the comments getting attached to unintended nodes & this introducing additional complexity to extract the meta..

For example, if the use case is "switch off the normalization of an identifier", we could have:

>>> sqlglot.parse_one("select a /* sqlglot.meta normalization = off */, b")
(SELECT expressions:
  (COLUMN this:
    (IDENTIFIER this: a, quoted: False), comments: [' sqlglot.meta normalization = off ']),
  (COLUMN this:
    (IDENTIFIER this: b, quoted: False)))

Note that the info is not set at the Identifier level here, but at the Column level, because of how comments are processed & attached in expressions at parse time. This makes it a bit awkward to detect the meta, because in normalize_identifier we'd have to check the parent to see if 1) it's a column and 2) has the meta set.

@barakalon
Copy link
Collaborator

What's the intended use of this?

@tobymao
Copy link
Owner Author

tobymao commented Sep 30, 2023

What's the intended use of this?

it provides an escape hatch for users to annotate their sql in some way. for example if a user doesn’t want something optimized

Copy link
Collaborator

@barakalon barakalon left a comment

Choose a reason for hiding this comment

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

Ah, interesting.

I agree with @georgesittas 's comment re: difficult to know exactly what node you're annotating. And SQLGlot's support for comments isn't great in general. But I wonder what nodes you'll actually need to annotate in practice - maybe its only things like Select and Table.

@georgesittas
Copy link
Collaborator

georgesittas commented Sep 30, 2023

But I wonder what nodes you'll actually need to annotate in practice - maybe its only things like Select and Table.

The motivating use case was disabling normalization for certain identifiers that are used to access JSON data, for example field in some_json.field. These identifiers are currently normalized according to the dialect's resolution rules, but JSON values don't necessarily follow these rules and so normalizing them could break the query.

@tobymao tobymao force-pushed the toby/set_meta branch 2 times, most recently from c8d66ea to 7c91edd Compare October 1, 2023 02:53
@tobymao tobymao merged commit 8dc2a9c into main Oct 2, 2023
@tobymao tobymao deleted the toby/set_meta branch October 2, 2023 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants