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

Add lazy evaluation of server_version_info #371

Merged
merged 1 commit into from
May 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions tests/integration/test_sqlalchemy_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import sqlalchemy as sqla
from sqlalchemy.sql import and_, not_, or_

from tests.integration.conftest import trino_version
from tests.unit.conftest import sqlalchemy_version
from trino.sqlalchemy.datatype import JSON

Expand Down Expand Up @@ -497,3 +498,24 @@ def test_get_view_names_raises(trino_connection):

with pytest.raises(sqla.exc.NoSuchTableError):
sqla.inspect(engine).get_view_names(None)


@pytest.mark.parametrize('trino_connection', ['system'], indirect=True)
@pytest.mark.skipif(trino_version() == '351', reason="version() not supported in older Trino versions")
def test_version_is_lazy(trino_connection):
_, conn = trino_connection
result = conn.execute(sqla.text("SELECT 1"))
result.fetchall()
num_queries = _num_queries_containing_string(conn, "SELECT version()")
assert num_queries == 0
version_info = conn.dialect.server_version_info
assert isinstance(version_info, tuple)
num_queries = _num_queries_containing_string(conn, "SELECT version()")
assert num_queries == 1


def _num_queries_containing_string(connection, query_string):
statement = sqla.text("select query from system.runtime.queries order by query_id desc offset 1 limit 1")
result = connection.execute(statement)
rows = result.fetchall()
return len(list(filter(lambda rec: query_string in rec[0], rows)))
23 changes: 14 additions & 9 deletions trino/sqlalchemy/dialect.py
Original file line number Diff line number Diff line change
Expand Up @@ -336,15 +336,20 @@ def has_sequence(self, connection: Connection, sequence_name: str, schema: str =
"""Trino has no support for sequence. Returns False indicate that given sequence does not exists."""
return False

def _get_server_version_info(self, connection: Connection) -> Any:
query = "SELECT version()"
try:
res = connection.execute(sql.text(query))
version = res.scalar()
return tuple([version])
except exc.ProgrammingError as e:
logger.debug(f"Failed to get server version: {e.orig.message}")
return None
@classmethod
def _get_server_version_info(cls, connection: Connection) -> Any:
def get_server_version_info(_):
query = "SELECT version()"
try:
res = connection.execute(sql.text(query))
version = res.scalar()
return tuple([version])
except exc.ProgrammingError as e:
logger.debug(f"Failed to get server version: {e.orig.message}")
return None

# Make server_version_info lazy in order to only make HTTP calls if user explicitly requests it.
cls.server_version_info = property(get_server_version_info, lambda instance, value: None)
Copy link
Member

Choose a reason for hiding this comment

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

Why not use the decorator @property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because server_version_info is not in our codebase but in sqlalchemy's, so we need to resort to this type of instantiation.

Copy link
Member

Choose a reason for hiding this comment

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

I mean why use the property() function instead of a decorator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have a look at sqlalchemy's code. Simply adding the property wouldn't work as it would be overwritten.

Copy link
Member

@nineinchnick nineinchnick May 13, 2023

Choose a reason for hiding this comment

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

Thanks, I get it now. But it seems we're doing a hack here where we override the server info property with one that has a getter for lazy evaluation, but a noop setter. Then we return a None, which sqlalchemy tries to assign to that prop, but that'll get ignored because go the setter. Is it worth adding a comment explaining this, if I got it right?


def _raw_connection(self, connection: Union[Engine, Connection]) -> trino_dbapi.Connection:
if isinstance(connection, Engine):
Expand Down