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

Open Beginner friendly
#75 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
1/5
Estimated time
Under an hour
Newbie friendliness
92/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
python, sqlalchemy
Domain
databases

Research direction

Start in src/databricks/sqlalchemy/base.py at get_columns(), then compare its reflection handling with get_pk_constraint() and the upstream dialects mentioned in the issue. Exercise repeated calls with the same info_cache and verify that the cache is populated and the warehouse round-trip occurs only once.

Written by the indexing model from the issue text.

Description

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.

Dominant language
Python
Stars
24
Forks
18
PR merge metrics
No merged PRs in 30d

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from databricks/databricks-sqlalchemy

All issues in databricks/databricks-sqlalchemy

Similar issues

More Python issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.