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

Don't store ddl on schema definitions #45772

Conversation

adrianna-chang-shopify
Copy link
Contributor

@adrianna-chang-shopify adrianna-chang-shopify commented Aug 5, 2022

Summary

We'll ensure that any consumers that require ddl obtain it by visiting
the schema definition. Consequently, we should also stop visiting the
schema definition in the "schema definition builder" methods -- callers
will need to both build a schema definition, and then visit it using a
SchemaCreation object. This means schema definitions will not be populated
with SQL type information, or any other information that is set when
the definition is visited.

This PR removes the ddl attribute from various schema definitions we'd added it to, and makes it so we're no longer visiting the definitions in the builder methods, but back in the schema statement methods where this was originally taking place.

Other Information

This change is being made because storing ddl on schema definitions subverts the intention of the Visitor pattern in its original form. We shouldn't be storing ddl on an object and allowing it to leak out to consumers -- instead, callers interacting with schema definition objects should be responsible for visiting the object themselves to produce ddl.

cc @matthewd @eileencodes

We'll ensure that any consumers that require ddl obtain it by visiting
the schema definition. Consequently, we should also stop visiting the
schema definition in the "schema definition builder" methods -- callers
will need to both build a schema definition, and then visit it using a
SchemaCreation object. This means schema definitions will not be populated
with SQL type information, or any other information that is set when
the definition is visited.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants