Skip to content

Conversation

lucianosrp
Copy link
Contributor

@lucianosrp lucianosrp commented Apr 18, 2024

This PR addresses two warnings coming from SqlAlchemy:

  • SADeprecationWarning when sqlalchemy version 2 when creating the engine
  • SAWarning because the attribute supports_statement_cache is not set

If I run this code on sqlalchemy 2.0.29 I get these warnings:

Sample code:

from sqlalchemy import create_engine, text
import urllib.parse

FIREBOLT_USERNAME = ...
FIREBOLT_PASSWORD = ...
DEFAULT_REGION = ...

SECRET = urllib.parse.quote_plus(FIREBOLT_PASSWORD)

database_name = ...
engine_name = ...
account_name = ... 
uri = f"firebolt://{FIREBOLT_USERNAME}:{SECRET}@{database_name}/{engine_name}?account_name={account_name}"

engine = create_engine(uri)
with engine.connect() as conn:
    conn.execute(text('SELECT 1;'))

Warnings


SADeprecationWarning: The dbapi() classmethod on dialect classes has been renamed to import_dbapi().  Implement an import_dbapi() classmethod directly on class <class 'firebolt_db.firebolt_dialect.FireboltDialect'> to remove this warning; the old .dbapi() classmethod may be maintained for backwards compatibility.
  engine = create_engine(uri)

SAWarning: Dialect firebolt:firebolt will not make use of SQL compilation caching as it does not set the 'supports_statement_cache' attribute to ``True``.  This can have significant performance implications including some performance degradations in comparison to prior SQLAlchemy versions.  Dialect maintainers should seek to set this attribute to True after appropriate development and testing for SQLAlchemy 1.4 caching support.   Alternatively, this attribute may be set to False which will disable this warning. (Background on this warning at: https://sqlalche.me/e/20/cprf)
  conn.execute(text('SELECT 1;'))

With the changes on this PR, these warnings will not be present.

@lucianosrp lucianosrp requested a review from a team as a code owner April 18, 2024 11:25
@ptiurin
Copy link
Collaborator

ptiurin commented Apr 19, 2024

Thank you for the contribution @lucianosrp ! I've run the integration tests and everything seems to be in order. The warnings are no longer present too.

@ptiurin ptiurin merged commit 9e1eec9 into firebolt-db:main Apr 19, 2024
@lucianosrp lucianosrp deleted the fix-warnings branch October 1, 2024 12:48
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.

2 participants