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.

Sign up for GitHub

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

Jump to bottom

feat(postgres): use psycopg rather than psycopg2 #10659

Merged
merged 8 commits into from
Jan 17, 2025
Merged

feat(postgres): use psycopg rather than psycopg2 #10659

merged 8 commits into from
Jan 17, 2025

Conversation

judahrand
Copy link
Contributor

@judahrand judahrand commented Jan 8, 2025 •
edited
Loading

Description of changes

The newer psycopg package generally has better performance and is more maintained than the older psycopg2 package.

Issues closed

@github-actions github-actions bot added tests Issues or PRs related to tests postgres The PostgreSQL backend ci Continuous Integration issues or PRs dependencies Issues or PRs related to dependencies labels Jan 8, 2025
@judahrand judahrand force-pushed the psycopg3 branch 3 times, most recently from 8d76d63 to 31312f3 Compare January 8, 2025 12:02
@judahrand judahrand closed this Jan 8, 2025
@judahrand judahrand reopened this Jan 8, 2025
@judahrand judahrand force-pushed the psycopg3 branch 9 times, most recently from 0345ad5 to 4c83730 Compare January 9, 2025 09:25
@github-actions github-actions bot added the risingwave The RisingWave backend label Jan 9, 2025
@judahrand judahrand changed the title feat(postgres): use psycopg rather than psycopg2 for Postgres feat(postgres): use psycopg rather than psycopg2 Jan 9, 2025
@judahrand judahrand force-pushed the psycopg3 branch 2 times, most recently from 2fa9f79 to 41c5082 Compare January 9, 2025 11:15
@judahrand
Copy link
Contributor Author

I was planning to continue this PR when I got the chance. I realized after starting that Rising Wave needed to move too because of the inheritance structure but that one is more difficult to iterate on in local development.

@gforsyth
Copy link
Member

I was planning to continue this PR when I got the chance. I realized after starting that Rising Wave needed to move too because of the inheritance structure but that one is more difficult to iterate on in local development.

@judahrand -- what kind of machine are you on? There's a risingwave container in our testing setup, so you should be able to just up risingwave (but this may be trickier if you're on an M1 or M2 mac)

@cpcloud cpcloud added this to the 10.0 milestone Jan 15, 2025
@github-actions github-actions bot added the nix Issues or PRs related to nix label Jan 15, 2025
@cpcloud
Copy link
Member

cpcloud commented Jan 15, 2025

If the main use case here is for the postgres backend, then I would suggest we decouple the backends first and then do the upgrade. I can put up a PR for that.

As is, even after fixing the top level risingwave integration there are some new, reproducible segfaults.

@cpcloud cpcloud mentioned this pull request Jan 15, 2025
Merged
gforsyth pushed a commit that referenced this pull request Jan 15, 2025
@cpcloud
refactor(risingwave): decouple the risingwave backend from the postgr…
bd30ba5 
…es backend (#10669)

Decouple the risingwave backend from the postgres backend to allow use
of `psycopg` (not `psycopg2`) with the postgres backend in #10659.
@cpcloud cpcloud force-pushed the psycopg3 branch 4 times, most recently from df06904 to 5214e45 Compare January 16, 2025 11:42
cpcloud
cpcloud reviewed Jan 17, 2025
View reviewed changes
ibis/backends/risingwave/__init__.py
@@ -106,48 +105,6 @@ def _from_url(self, url: ParseResult, **kwargs):

return self.connect(**kwargs)

def _register_in_memory_table(self, op: ops.InMemoryTable) -> None:
Copy link
Member

@cpcloud cpcloud Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This was a duplicated definition not caught by ruff that I introduced in #10669.

@cpcloud
Copy link
Member

cpcloud commented Jan 17, 2025

Thanks @judahrand for kicking this off!

@cpcloud cpcloud enabled auto-merge (squash) January 17, 2025 08:48
@cpcloud cpcloud merged commit 85b449e into ibis-project:main Jan 17, 2025
88 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpcloud cpcloud cpcloud approved these changes

Assignees
No one assigned
Labels
ci Continuous Integration issues or PRs dependencies Issues or PRs related to dependencies nix Issues or PRs related to nix postgres The PostgreSQL backend risingwave The RisingWave backend tests Issues or PRs related to tests
Projects
None yet
Milestone
10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@judahrand @gforsyth @cpcloud