Skip to content

get_columns() missing @reflection.cache causes a warehouse round-trip on every reflection call #75

Description

@TangoEnSkai

Summary

DatabricksDialect.get_columns() is missing the @reflection.cache decorator, so it never participates in SQLAlchemy's reflection cache. Every call issues a fresh GetColumns round-trip to the warehouse, however many times the same table is reflected through the same Inspector.

This is the sibling of #72 (get_foreign_keys()), which is addressed in #74. get_columns() is a separate occurrence of the same omission and is not covered by that PR.

Expected behaviour

Like get_pk_constraint(), has_table(), get_table_names(), get_view_names(), get_materialized_view_names(), get_temp_view_names(), get_schema_names() and get_table_comment() — and like get_columns() in SQLAlchemy's own SQLite, PostgreSQL and MySQL dialects, all three of which are decorated — repeated reflection of the same table through one Inspector should cost one round-trip, not one per call.

Inspector.get_columns() explicitly threads the cache through to the dialect (sqlalchemy/engine/reflection.py):

with self._operation_context() as conn:
    col_defs = self.dialect.get_columns(
        conn, table_name, schema, info_cache=self.info_cache, **kw
    )

The info_cache kwarg arrives, lands in **kwargs, and is discarded.

Actual behaviour

Reproduced against main (SQLAlchemy 2.0.52), stubbing out the transport so the call count is directly observable:

info_cache = {}
for _ in range(3):
    dialect.get_columns(conn, "t", None, info_cache=info_cache)
method calls server round-trips info_cache keys
get_columns() 3 3 [] — never populated
get_pk_constraint() 3 1 [('get_pk_constraint', ('t',), (('schema', None),))]

Note the cost is not always a single statement: when cur.columns() returns an empty list, get_columns() follows up with DESCRIBE TABLE EXTENDED to distinguish a genuinely column-less table from a missing one (base.py:156). For such tables an uncached call is two round-trips, repeated every time.

Callers that reuse one Inspector across many lookups feel this directly — Alembic's autogenerate is the common case, as is any long-lived application that reflects per request.

Root cause

src/databricks/sqlalchemy/base.py:139get_columns() lacks @reflection.cache. Compare get_pk_constraint() at line 212, which is decorated.

Fix

Add @reflection.cache to get_columns().

Two things worth noting for whoever picks this up, both checked:

  • Caching is safe with respect to Inspector._instantiate_types(), which mutates the returned column dicts in place. It is guarded by if not isinstance(coltype, TypeEngine), so re-running it over an already-instantiated cached list is a no-op. This is the same situation the upstream dialects are in.
  • get_indexes() is also undecorated but returns the EMPTY_INDEX constant without touching the server, so it needs no cache. get_columns() is the only remaining method where the omission costs a round-trip.

I'm happy to open a PR for this if it's welcome.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions