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

joins support #53

Merged
merged 1 commit into from
Jul 19, 2019
Merged

joins support #53

merged 1 commit into from
Jul 19, 2019

Conversation

antonio-antuan
Copy link
Contributor

OK, here is a join support for standard sqla-core (issue) and some kind of joins-api refactor.

Example is here: tests.sql.test_schema.SchemaTestCase#test_reflect.

The magic is here: clickhouse_sqlalchemy.drivers.base.ClickHouseDialect#reflecttable.
I replace table bounded with metadata by the table with another class.

The place that can make you sad is clickhouse_sqlalchemy.orm.query.Query#join. But I hope it is not: can't invent another way to make this possible: there are a lot of work inside of sqlalchemy.orm.query.Query#join method which should be done, but in the and another class must be used.

@xzkostyan xzkostyan merged commit 0d13389 into xzkostyan:master Jul 19, 2019
@xzkostyan
Copy link
Owner

Very sorry for late review.

@hhell
Copy link
Contributor

hhell commented Jul 29, 2019

I replace table bounded with metadata by the table with another class

What exactly is the point and is there a test that should fail without the magic?

(currently, test_reflect fails for me in either case)

hhell added a commit to hhell/clickhouse-sqlalchemy that referenced this pull request Jul 29, 2019
@hhell
Copy link
Contributor

hhell commented Jul 29, 2019

What exactly is the point and is there a test that should fail without the magic?

After a bit more digging:

  • Tests failed because the table was created in database default and was not found in the database test
  • The magic is necessary for metadata.reflect to make a clickhouse_sqlalchemy.sql.Table instance instead of sqlalchemy.sql.schema.Table, for the extra parameters support in join
  • The magic was breaking the sqlalchemy.sql.schema.Table(..., autoload=True) use case (the returned table had empty schema)

I put an improvement (support and test of both use cases) into a commit in #62

hhell added a commit to hhell/clickhouse-sqlalchemy that referenced this pull request Aug 12, 2019
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.

None yet

3 participants