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

Dialect: Add support for asyncpg and psycopg3 drivers #11

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amotl
Copy link
Member

@amotl amotl commented Dec 23, 2023

About

This patch adds support for asyncpg and psycopg3 drivers, by introducing the crate+asyncpg:// and crate+psycopg:// dialect identifiers.

It also adds the crate+urllib3:// dialect identifier, for explicitly addressing the urllib3-based transport, in case there might be other HTTP-based transport adapters in the future, using libraries like aiohttp, httpx, or niquests. 1

Notes

  • The asynchronous variant of psycopg is also supported and will be automatically selected when using create_async_engine() instead of create_engine(), so it doesn't have a dedicated dialect identifier.

  • All of this will only work with SQLAlchemy 2.x.

Installation

pip install 'sqlalchemy-cratedb[all] @ git+https://github.com/crate/sqlalchemy-cratedb@amo/postgresql-async'

References

Backlog

Footnotes

  1. Picked up from another discussion at https://github.com/panodata/grafana-client/issues/134.

@amotl amotl force-pushed the amo/postgresql-async branch 2 times, most recently from 20ab16e to e1929cb Compare December 23, 2023 01:36
Comment on lines 24 to 43
def test_engine_sync_vanilla():
"""
crate:// -- Verify connectivity and data transport with vanilla HTTP-based driver.
"""
engine = sa.create_engine("crate://crate@localhost:4200/", echo=True)
assert isinstance(engine, sa.engine.Engine)
with engine.connect() as connection:
result = connection.execute(QUERY)
assert result.mappings().fetchone() == {'mountain': 'Acherkogel', 'coordinates': [10.95667, 47.18917]}


def test_engine_sync_urllib3():
"""
crate+urllib3:// -- Verify connectivity and data transport *explicitly* selecting the HTTP driver.
"""
engine = sa.create_engine("crate+urllib3://crate@localhost:4200/", isolation_level="AUTOCOMMIT", echo=True)
assert isinstance(engine, sa.engine.Engine)
with engine.connect() as connection:
result = connection.execute(QUERY)
assert result.mappings().fetchone() == {'mountain': 'Acherkogel', 'coordinates': [10.95667, 47.18917]}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the reference record representation.

{'mountain': 'Acherkogel', 'coordinates': [10.95667, 47.18917]}

Comment on lines 46 to 67
def test_engine_sync_psycopg():
"""
crate+psycopg:// -- Verify connectivity and data transport using the psycopg driver (version 3).
"""
engine = sa.create_engine("crate+psycopg://crate@localhost:5432/", isolation_level="AUTOCOMMIT", echo=True)
assert isinstance(engine, sa.engine.Engine)
with engine.connect() as connection:
result = connection.execute(QUERY)
assert result.mappings().fetchone() == {'mountain': 'Acherkogel', 'coordinates': '(10.95667,47.18917)'}


@pytest.mark.asyncio
async def test_engine_async_psycopg():
"""
crate+psycopg:// -- Verify connectivity and data transport using the psycopg driver (version 3).
This time, in asynchronous mode.
"""
engine = create_async_engine("crate+psycopg://crate@localhost:5432/", isolation_level="AUTOCOMMIT", echo=True)
assert isinstance(engine, AsyncEngine)
async with engine.begin() as conn:
result = await conn.execute(QUERY)
assert result.mappings().fetchone() == {'mountain': 'Acherkogel', 'coordinates': '(10.95667,47.18917)'}
Copy link
Member Author

@amotl amotl Dec 23, 2023

Choose a reason for hiding this comment

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

When using psycopg, there are deviations, which clearly need to be addressed. By chance, you might have seen this already, @SStorm? Or did you only use asyncpg?

{'mountain': 'Acherkogel', 'coordinates': '(10.95667,47.18917)'}

Comment on lines 70 to 81
@pytest.mark.asyncio
async def test_engine_async_asyncpg():
"""
crate+asyncpg:// -- Verify connectivity and data transport using the asyncpg driver.
This exclusively uses asynchronous mode.
"""
from asyncpg.pgproto.types import Point
engine = create_async_engine("crate+asyncpg://crate@localhost:5432/", isolation_level="AUTOCOMMIT", echo=True)
assert isinstance(engine, AsyncEngine)
async with engine.begin() as conn:
result = await conn.execute(QUERY)
assert result.mappings().fetchone() == {'mountain': 'Acherkogel', 'coordinates': Point(10.95667, 47.18917)}
Copy link
Member Author

@amotl amotl Dec 23, 2023

Choose a reason for hiding this comment

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

When using asyncpg, the response record is not wrong, but its shape is different, as it provides a dedicated Point type for representing coordinates.

{'mountain': 'Acherkogel', 'coordinates': Point(10.95667, 47.18917)}

@amotl amotl changed the title Add support for psycopg and asyncpg drivers Add support for asyncpg and psycopg drivers Dec 23, 2023
@amotl amotl changed the title Add support for asyncpg and psycopg drivers Add support for asyncpg and psycopg3 drivers Dec 23, 2023
@amotl amotl requested a review from matriv December 23, 2023 23:33
@amotl amotl mentioned this pull request Jan 12, 2024
2 tasks
@amotl amotl changed the title Add support for asyncpg and psycopg3 drivers Dialect: Add support for asyncpg and psycopg3 drivers Jan 15, 2024
@amotl amotl force-pushed the amo/fix-inspector branch 5 times, most recently from 6ac0a22 to d2c7613 Compare June 13, 2024 14:33
Base automatically changed from amo/fix-inspector to main June 13, 2024 14:38
@amotl amotl force-pushed the amo/postgresql-async branch 3 times, most recently from 5c906ad to 474f658 Compare June 14, 2024 02:20
Comment on lines +403 to +412
When using the PostgreSQL protocol with drivers `psycopg` or `asyncpg`,
the paramstyle is not `qmark`, but `pyformat`.
Copy link
Member

Choose a reason for hiding this comment

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

I am not quite following. I assume we would want to use server side binding (i.e. qmark)?

Copy link
Member Author

@amotl amotl Jun 25, 2024

Choose a reason for hiding this comment

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

This is probably the reason for needing the workaround at all: Because PostgreSQL drivers psycopg and asyncpg, or the SA dialect, use pyformat, but CrateDB uses qmark, we may need to adjust, iirc.

At least, the patch in its current shape needs it. Maybe there are alternatives to implement it, possibly even easier ones. We will be happy to learn about them.

@cla-bot cla-bot bot added the cla-signed label Oct 1, 2024
This introduces the `crate+psycopg://`, `crate+asyncpg://`, and
`crate+urllib3://` dialect identifiers. The asynchronous variant of
`psycopg` is also supported.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants