Allow foreign table constraint without columns #1608
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Postgres makes skipping the `(referenced-columns)` part in `ALTER TABLE ... FOREIGN KEY .. REFERENCES <tablename> (referenced-columns)` per https://www.postgresql.org/docs/current/sql-createtable.html#SQL-CREATETABLE-PARMS-REFERENCES. Since the generic SQL grammar requires referenced columns (https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#referential-constraint-definition), this change applies only to Postgres.
|
An alternative design would be to add a new method in the |
iffyio
reviewed
Dec 18, 2024
src/parser/mod.rs
Outdated
| let referred_columns = self.parse_parenthesized_column_list(Mandatory, false)?; | ||
| // PostgreSQL allows foreign key columns to be optional | ||
| // (https://www.postgresql.org/docs/current/sql-createtable.html#SQL-CREATETABLE-PARMS-REFERENCES) | ||
| let parenthesized_column_list_optional = if dialect_of!(self is PostgreSqlDialect) { |
There was a problem hiding this comment.
Since this behavior is a superset of the previous I think we can do without the dialect check and always set to Optional?
Also could we add a test case to sqlparse_common.rs demonstrating the new behavior?
There was a problem hiding this comment.
Good point. Made the change.
Also, fixes DDL generation when reference columns aren't specified and adds a test.
ramnivas
added a commit
to exograph/exograph
that referenced
this pull request
Dec 19, 2024
Our migration tests have used strings in Rust files (for exo and expected SQL), which required string escaping, prefix stripping, and other modifications. This made it difficult to read and extend the test code (for example, consider `MigrationScope`s). Also, we relied on exact string matches, which required paying too much attention to the precise format. This refactoring: 1. Move .exo and .sql files outside of rust code 2. Use `sqlparser` to match without considering the format Note that in some cases, `sqlparser` doesn't parse valid syntax, so we rely on exact string comparison as a fallback/optimization. We have already contributed a couple of changes (apache/datafusion-sqlparser-rs#1608, apache/datafusion-sqlparser-rs#1610) and will continue to do so to remove this reliance.
|
Thank you for keeping up the review train @iffyio -- being part of the unsung heros of the Rust data ecosystem 🦾 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Postgres allows skipping the
(referenced-columns)part inALTER TABLE ... FOREIGN KEY .. REFERENCES <tablename> (referenced-columns)per https://www.postgresql.org/docs/current/sql-createtable.html#SQL-CREATETABLE-PARMS-REFERENCES.Since the generic SQL grammar requires referenced columns (https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#referential-constraint-definition), this change applies only to Postgres.