Skip to content

fix: add the two missing @reflection.cache decorators (get_foreign_keys, get_columns) - #74

Open
TangoEnSkai wants to merge 2 commits into
databricks:mainfrom
TangoEnSkai:fix/cache-get-foreign-keys
Open

TangoEnSkai wants to merge 2 commits into
databricks:mainfrom
TangoEnSkai:fix/cache-get-foreign-keys

Conversation

@TangoEnSkai

@TangoEnSkai TangoEnSkai commented Aug 23, 2026 •

Copy link
Copy Markdown

Context

SQLAlchemy threads an info_cache dict through every Inspector call so that
reflecting the same table repeatedly costs one round-trip rather than one per
call. A dialect method only participates in that cache when it is decorated with
@reflection.cache.

Two methods in DatabricksDialect were missing it — get_foreign_keys() (#72)
and get_columns() (#75). Both issue a warehouse round-trip on every single
call. Their neighbours 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() are all decorated, which makes
these look like omissions rather than deliberate exclusions.

Inspector.get_columns() shows the kwarg being handed over explicitly:

col_defs = self.dialect.get_columns(
    conn, table_name, schema, info_cache=self.info_cache, **kw
)

It arrived, landed in **kwargs, and was discarded.

Measured against main with the transport stubbed out, three calls sharing one
info_cache:

method server round-trips info_cache keys
get_columns() 3 [] — never populated
get_foreign_keys() 3 [] — never populated
get_pk_constraint() 1 populated

For get_columns() the cost is not always a single statement: when
cur.columns() returns an empty list, it follows up with DESCRIBE TABLE EXTENDED to tell a column-less table from a missing one (base.py:156), so an
uncached call on such a table 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.

What

  • Add @reflection.cache to get_foreign_keys() and get_columns().
  • Add tests/test_local/test_reflection_cache.py covering, for both methods: the
    cache hit, that a direct call passing no info_cache is unaffected (the
    decorator falls straight through when the kwarg is absent, so callers invoking
    the dialect method directly keep today's behaviour), per-table cache keying for
    get_foreign_keys(), and a regression guard on the already-correct
    get_pk_constraint().

get_indexes() is left undecorated on purpose: it returns the EMPTY_INDEX
constant without touching the server, so a cache would buy nothing.

Why

An uncached reflection method is a warehouse query per table per call. The change
is the same mechanism the neighbouring methods already use, and it matches
upstream practice — SQLAlchemy's own SQLite, PostgreSQL and MySQL dialects all
decorate get_columns().

It cannot change results: @reflection.cache is a no-op unless SQLAlchemy itself
supplies info_cache, and the calls it collapses are identical by construction.
Caching is also 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.

Scope note

reflection.cache builds its key from fn.__name__, so this does not make
get_pk_constraint() and get_foreign_keys() share a single DESCRIBE TABLE EXTENDED within one reflection pass — those two still issue the same statement
once each. Removing that remaining duplication would mean caching
_describe_table_extended() itself, which is a larger change and a different
discussion; happy to open a follow-up issue if you'd like it pursued.

Note on scope of this PR

This started as the get_foreign_keys() fix for #72. While verifying it I found
get_columns() had the identical omission and filed #75. Since both are the same
one-line change to the same file and share one test module, keeping them apart
would have meant two PRs conflicting on all three files — so they are together
here. Happy to split them if you'd rather review them separately.

Completion Criteria

  • @reflection.cache added to get_foreign_keys() and get_columns()
  • Unit tests for the cache hit, per-table keying, and the no-info_cache path
  • Reverting either decorator fails exactly that method's cache test and nothing else
  • pytest tests/test_local (offline modules) — 300 passed
  • mypy --install-types --non-interactive src — clean (matches the CI check)
  • black --check clean on both changed files (_parse.py is flagged only by a
    newer local black than the pinned ^22.3.0, is untouched here, and is
    already flagged on main)
  • CHANGELOG entries under a new # Unreleased section
  • Commits signed off (DCO)

close #72
close #75

get_foreign_keys() was the only _describe_table_extended()-backed
reflection method without @reflection.cache. SQLAlchemy threads an
info_cache dict through every Inspector call so that repeated reflection
of the same table costs one round-trip rather than one per call, but a
dialect method only participates when it is decorated.

Undecorated, every get_foreign_keys() call issued a fresh

    DESCRIBE TABLE EXTENDED `catalog`.`schema`.`table`

against the warehouse, however many times the same table was reflected
through the same Inspector. For an application that reflects on each
request, that is a warehouse query per table per request.

get_pk_constraint() sits directly above it, uses the same helper, and is
decorated; get_table_names(), get_view_names(), has_table(),
get_schema_names() and get_table_comment() are decorated too. This was an
omission rather than a deliberate exclusion.

Note the cache key includes fn.__name__, so this does not make
get_pk_constraint() and get_foreign_keys() share one DESCRIBE within a
single pass — that duplication is separate and would need the helper
itself to be cached.

Add unit tests covering the cache hit, that the cache is keyed per table,
that a direct call passing no info_cache is unaffected, and a regression
guard on the already-correct get_pk_constraint().

Resolves databricks#72

Signed-off-by: TangoEnSkai <21152231+TangoEnSkai@users.noreply.github.com>
get_columns() was the second dialect method missing @reflection.cache,
so it never participated in the cache SQLAlchemy threads through
Inspector.get_columns():

    col_defs = self.dialect.get_columns(
        conn, table_name, schema, info_cache=self.info_cache, **kw
    )

The info_cache kwarg arrived, landed in **kwargs, and was discarded.
Measured against main, three calls sharing one info_cache produced three
GetColumns round-trips and left info_cache empty, where the decorated
get_pk_constraint() produced one.

The cost is not always a single statement: when cur.columns() returns an
empty list, get_columns() follows up with DESCRIBE TABLE EXTENDED to tell
a column-less table from a missing one, so an uncached call on such a
table is two round-trips, repeated every time.

SQLAlchemy's own SQLite, PostgreSQL and MySQL dialects all decorate
get_columns(), so this matches upstream practice rather than inventing
one. Caching is safe with respect to Inspector._instantiate_types(),
which mutates the returned column dicts in place but is guarded by
'if not isinstance(coltype, TypeEngine)' and is therefore a no-op on an
already-instantiated cached list.

get_indexes() remains undecorated deliberately: it returns the
EMPTY_INDEX constant without touching the server.

Resolves databricks#75

Signed-off-by: TangoEnSkai <21152231+TangoEnSkai@users.noreply.github.com>
@TangoEnSkai TangoEnSkai changed the title fix: cache get_foreign_keys() to stop redundant DESCRIBE TABLE EXTENDED fix: add the two missing @reflection.cache decorators (get_foreign_keys, get_columns) Aug 23, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant