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

Ignore primary key, foreign key and unique constraints for DDL generation #541

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

willmostly
Copy link

@willmostly willmostly commented Mar 18, 2025

Description

Define custom visitors in TrinoDDLCompiler to ignore constraints.

Non-technical explanation

By default, SQLAlchemy adds PRIMARY KEY and other constraint keywords to table creation DDL when these constraints are defined on a Table or class derived from declarative base. Since Trino does not support constraints, this results in invalid SQL. With this change the trino sqlalchemy extension now ignores primary key, foreign key and unique constraints when generating table DDL.

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

* ignore constraints for table DDL generation

@cla-bot cla-bot bot added the cla-signed label Mar 18, 2025
@willmostly willmostly force-pushed the will/ignore_ddl_constraints branch from e3ace59 to 66c389f Compare March 18, 2025 15:03
@willmostly willmostly force-pushed the will/ignore_ddl_constraints branch from 66c389f to f7d4d73 Compare March 18, 2025 15:05
@hashhar hashhar requested a review from damian3031 April 10, 2025 09:18
Comment on lines +184 to +185
def visit_foreign_key_constraint(self, constraint, **kw):
return None
Copy link
Member

@damian3031 damian3031 Apr 11, 2025

Choose a reason for hiding this comment

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

Rather than silently ignoring constraints, maybe we should raise an explicit error indicating that these constraints are not supported in Trino:

Suggested change
def visit_foreign_key_constraint(self, constraint, **kw):
return None
from sqlalchemy.exc import CompileError
...
def visit_foreign_key_constraint(self, constraint, **kw):
raise CompileError("Trino does not support FOREIGN KEY constraints.")

Or at least we could add a warning to make it clearer that the constraint is being ignored:

Suggested change
def visit_foreign_key_constraint(self, constraint, **kw):
return None
import warnings
from sqlalchemy.exc import SAWarning
...
def visit_foreign_key_constraint(self, constraint, **kw):
warnings.warn("Trino does not support FOREIGN KEY constraints. Constraint will be ignored.", SAWarning)
return None

@willmostly @hashhar WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants