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

Refactor POST_CREATE and POST_EXPRESSION properties #1219

Merged
merged 11 commits into from
Feb 23, 2023
Merged

Refactor POST_CREATE and POST_EXPRESSION properties #1219

merged 11 commits into from
Feb 23, 2023

Conversation

treysp
Copy link
Collaborator

@treysp treysp commented Feb 23, 2023

Convert Create args to properties in POST_CREATE location:

  • set
  • multiset
  • global_temporary
  • volatile
  • temporary
  • transient
  • external
  • materialized

Convert Create args to properties in POST_EXPRESSION location:

  • data
  • statistics
  • no_primary_index

Add preserve_rows property in POST_EXPRESSION location for ON COMMIT PRESERVE ROWS #1205

Make SetProperty unsupported in Snowflake (Teradata "CREATE MULTISET TABLE a" -> Snowflake "CREATE TABLE a") #1205

@treysp
Copy link
Collaborator Author

treysp commented Feb 23, 2023

The Teradata syntax "CREATE VOLATILE TABLE" isn't supported in Snowflake, and the "obvious" way to handle is to set the volatility property to unsupported in Snowflake. However, the volatility property is also mapped to keyword IMMUTABLE (which snowflake does support).

You can see in

"DETERMINISTIC": lambda self: self.expression(
that the VolatilityProperty is capturing all of deterministic, immutable, stable, and volatile.

Not sure the best way to handle this. We can pull them into separate properties, but that will complicate transpiling one of those words to another.

@treysp
Copy link
Collaborator Author

treysp commented Feb 23, 2023

I also see that the CREATE_TRANSIENT flag was essentially playing the role of UNSUPPORTED properties for the Transient property in POST_CREATE location.

I don't see this being used in any other dialects, so I'll remove it when responding to review comments.

# Whether 'CREATE ... TRANSIENT ... TABLE' is allowed

CREATE_TRANSIENT = True

sqlglot/dialects/spark.py Outdated Show resolved Hide resolved
sqlglot/parser.py Outdated Show resolved Hide resolved
@georgesittas
Copy link
Collaborator

The Teradata syntax "CREATE VOLATILE TABLE" isn't supported in Snowflake, and the "obvious" way to handle is to set the volatility property to unsupported in Snowflake. However, the volatility property is also mapped to keyword IMMUTABLE (which snowflake does support).

You can see in

"DETERMINISTIC": lambda self: self.expression(

that the VolatilityProperty is capturing all of deterministic, immutable, stable, and volatile.

Not sure the best way to handle this. We can pull them into separate properties, but that will complicate transpiling one of those words to another.

Maybe you can also check if the string captured in exp.VolatilityProperty is legal in Snowflake. So, for instance, you'd only allow IMMUTABLE if that's the only valid one.

@treysp treysp changed the title Refactor POST_CREATE properties Refactor POST_CREATE and POST_EXPRESSION properties Feb 23, 2023
@treysp
Copy link
Collaborator Author

treysp commented Feb 23, 2023

The Teradata syntax "CREATE VOLATILE TABLE" isn't supported in Snowflake, and the "obvious" way to handle is to set the volatility property to unsupported in Snowflake. However, the volatility property is also mapped to keyword IMMUTABLE (which snowflake does support).
You can see in

"DETERMINISTIC": lambda self: self.expression(

that the VolatilityProperty is capturing all of deterministic, immutable, stable, and volatile.
Not sure the best way to handle this. We can pull them into separate properties, but that will complicate transpiling one of those words to another.

Maybe you can also check if the string captured in exp.VolatilityProperty is legal in Snowflake. So, for instance, you'd only allow IMMUTABLE if that's the only valid one.

That makes sense, but I don't think it would work if multiple volatility words are used for different things in the same dialect and/or word usages aren't aligned across dialects. (So in one dialect VOLATILE means X and IMMUTABLE means Y, but in another dialect VOLATILE means Y and IMMUTABLE means X.)

I think resolving this will probably require reviewing volatility-related words across the dialects. I'm happy to do that, but it will take me a few days to get to it.

self._match_text_seq("COMMIT", "PRESERVE", "ROWS")
return exp.OnCommitProperty()

# def _parse_unique_property(self) -> exp.Expression:
Copy link
Owner

Choose a reason for hiding this comment

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

delete this

@tobymao tobymao merged commit e35b4db into tobymao:main Feb 23, 2023
@treysp treysp deleted the properties branch February 24, 2023 15:30
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