visit_primary_key_constraint() fails table creation with pk and fk

[SomeAkk]

Describe the bug
When make sql CREATE query for table with ForeignKey and primary_key both we can get wrong query, which contains unneeded comma.

class UserCluster(utl_ch.Base):
    __tablename__ = 'user_clusters'
    __table_args__ = (engines.MergeTree(order_by='id'),)

    id = Column(String(32), primary_key=True)
    request_id = Column(String(32), ForeignKey('clusterization_requests.id'))
    cluster_id = Column(String)
 
Base = get_declarative_base()
engine = create_engine('clickhouse://test:test@localhost:62180/test')
Base.metadata.create_all(engine)

create_all gives wrong SQL query:

CREATE TABLE user_clusters (
id FixedString(32),
request_id FixedString(32),
cluster_id String,
,          <- wrong comma
FOREIGN KEY(request_id) REFERENCES clusterization_requests (id)
) ENGINE = MergeTree()
ORDER BY id

which then fall all table creation in clickhouse by error:

>           raise DatabaseException(orig)
E           clickhouse_sqlalchemy.exceptions.DatabaseException: Orig exception: 
E           Code: 62. DB::Exception: Syntax error: failed at position 149 (',') (line 8, col 2): 
E           , 
E           FOREIGN KEY(request_id) REFERENCES clusterization_requests (id)
E           ) ENGINE = MergeTree()
E            ORDER BY id
E           
E           . Expected one of: table property (column, index, constraint) declaration, INDEX, CONSTRAINT, PROJECTION, PRIMARY KEY, column declaration, identifier. (SYNTAX_ERROR) (version 22.6.4.35 (official build))

To Reproduce

class UserCluster(utl_ch.Base):
    __tablename__ = 'user_clusters'
    __table_args__ = (engines.MergeTree(order_by='id'),)

    id = Column(String(32), primary_key=True)
    request_id = Column(String(32), ForeignKey('clusterization_requests.id'))
    cluster_id = Column(String)
 
Base = get_declarative_base()
engine = create_engine('clickhouse://test:test@localhost:62180/test')
Base.metadata.create_all(engine)

After create_all, when creating all tables, code create list of strings of constraints in sqlalchemy.sql.compiller.create_table_constraints(). That list contains empty string which goes from visit_primary_key_constraint() which for primary key returns '' (empty string):

    #.../clickhouse_sqlalchemy/drivers/compilers/ddlcompiler.py, line 80
    def visit_primary_key_constraint(self, constraint, **kw):
        # Do not render PKs.
        return ''

By code from sqlalchemy.sql.compiller.create_table_constraints(), line 4507, looks like we need to return None from visit_primary_key_constraint() instead empty string to not to make unneeded coma, change code to:

    #.../clickhouse_sqlalchemy/drivers/compilers/ddlcompiler.py, line 80
    def visit_primary_key_constraint(self, constraint, **kw):
        # Do not render PKs.
        return None

Expected behavior
No comma in sql query.

Versions

• SqlAlchemy 1.4.40
• Version of package 0.2.2
• Python 3.9

[xzkostyan]

Why do you use foreign keys? ClickHouse doen't have this feature.

[SomeAkk]

Why do you use foreign keys? ClickHouse doen't have this feature.

We use graphql, it let us get models with submodels with submodels with etc... from ch database.
To make it, graphql work with sqlalchemy's tables metadata - it use relationships, which use foreign keys. By that graphql know how to got data from model A and its submodel model B from db (it creates select query or two select, maybe subqueries - realy do not know, but it work).

All was good, while i not start to use our tables (and it's relationships, which use foreign keys) for tables creation for testing. Now, if continue strategy of using current tables, i understood, that i or create tables for tests, or specify a foreign key for the relationship - together they will lead to errors.

Totally, with fix in visit_primary_key_constraint (about what i wrote earlier) and creating some additional visitor which will work for fk like it must work for pk:

def visit_foreign_key_constraint(self, constraint, **kw):
        return None

I thing all will be ok.

Show me the code
In real, table definitions be fully with 2d table (code from our project):

class UserCluster(utl_ch.Base):
    __tablename__ = 'user_clusters'
    __table_args__ = (engines.MergeTree(order_by='id'),)

    id = Column(String(32), primary_key=True)
    request_id = Column(String(32), ForeignKey('clusterization_requests.id'))
    cluster_id = Column(String)


class ClusterizationRequest(utl_ch.Base):
    __tablename__ = 'clusterization_requests'
    __table_args__ = (engines.MergeTree(order_by='id'),)

    id = Column(String(32), primary_key=True)
    run_date = Column(DateTime)
    clusters = relationship('UserCluster')

Code above is One To Many pattern for relationships in sqlalchemy which used for graphql.
Also "looks like that example" can be finded in graphql python library graphene: defining models with relationships

[xzkostyan]

Version 0.2.3 with fix is released.