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

fix: fix select.join #284

Merged
merged 1 commit into from
Mar 14, 2024
Merged

fix: fix select.join #284

merged 1 commit into from
Mar 14, 2024

Conversation

Net-Mist
Copy link
Contributor

We discover than requests like select(table1).join(table2, table2.id == table1.table2Id).where([...])
didn't work with clickhouse-sqlalchemy but did work with standard sqlalchemy because select(table1).join(table2, table2.id == table1.table2Id) is a Select object in sqlalchemy2.

This PR fix this (just changed the return of the join method) and add a test to prove the fix works.

The diff in the test is massive because I added a for loop to test both ways to compute the join statement. Let me know if you think there is a better way to write the test

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Ensure PR doesn't contain untouched code reformatting: spaces, etc.
  • Run flake8 and fix issues.
  • Run pytest no tests failed. See https://clickhouse-sqlalchemy.readthedocs.io/en/latest/development.html.

@coveralls
Copy link

Coverage Status

coverage: 85.897% (+0.07%) from 85.824%
when pulling a71f687 on Net-Mist:fix_join
into 46482bf on xzkostyan:master.

@Net-Mist
Copy link
Contributor Author

Hello @xzkostyan
Now that the other PR has been merged, the unit-tests for this one are all green. Let me know if you think I should change something here

@xzkostyan xzkostyan merged commit 7c05244 into xzkostyan:master Mar 14, 2024
79 checks passed
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