Skip to content

refactor(ingest/mssql): centralize is_discovered_table cache-and-return#18122

Open
acrylJonny wants to merge 6 commits into
masterfrom
refactor/mssql-is-discovered-table-cache-helper
Open

refactor(ingest/mssql): centralize is_discovered_table cache-and-return#18122
acrylJonny wants to merge 6 commits into
masterfrom
refactor/mssql-is-discovered-table-cache-helper

Conversation

@acrylJonny

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to the DRY cleanup flagged in review on #17463 (thread).

  • Extracts an uncached _compute_is_discovered_table helper so each branch simply returns its verdict.
  • is_discovered_table now owns the single cache-and-return, removing the ~8x repeated result = ...; self._discovered_table_cache[name] = result; return result epilogue.
  • Eliminates the risk of a future branch forgetting to populate the cache.

Behavior is unchanged — pure refactor.

Note: this PR is stacked on top of fix/mssql-2-part-name-temp-table-filtering (#17463). Rebase onto master once that merges.

Test plan

  • pytest tests/unit/test_mssql.py — all 34 tests pass (existing is_discovered_table coverage exercises every branch)
  • ./gradlew :metadata-ingestion:lintFix clean

Made with Cursor

acrylJonny and others added 3 commits May 15, 2026 13:20
…covered_datasets

MSSQL stored-procedure bodies frequently reference tables as
`schema.table` even though discovery records them fully qualified
as `db.schema.table`. Previously, `is_discovered_table` treated any
name with fewer than `MSSQL_QUALIFIED_NAME_PARTS` segments as
undiscovered, which caused the temp-table filter to drop real
lineage edges from procedures that use 2-part references.

This adds a lookup pass for 2-part names: when `schema.table`
matches the trailing two segments of a fully-qualified entry in
`discovered_datasets`, treat the reference as a real table. 1-part
names (and 2-part names that don't match anything) keep the
previous undiscovered behaviour.

Adds unit-test coverage for the match and no-match cases on top of
the existing `is_discovered_table` test.

Co-authored-by: Cursor <cursoragent@cursor.com>
…ests

Address review feedback: collapse the trailing-segment match to a single
endswith, document the intentional cross-DB first-match-wins behavior and
pattern-filtering asymmetry, and add tests locking cross-DB matching and
temp-filter precedence for 2-part references.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions github-actions Bot added the ingestion PR or Issue related to the ingestion of metadata label Jul 1, 2026
@acrylJonny acrylJonny force-pushed the refactor/mssql-is-discovered-table-cache-helper branch from 60fe6f9 to fdc9bc8 Compare July 1, 2026 14:05
Extract an uncached _compute_is_discovered_table helper so each branch just
returns its verdict, and let is_discovered_table own the single
cache-and-return. Removes the ~8x repeated
`result = ...; self._discovered_table_cache[name] = result; return result`
epilogue and the risk of a future branch forgetting to populate the cache.
Behavior is unchanged.

Co-authored-by: Cursor <cursoragent@cursor.com>
@acrylJonny acrylJonny force-pushed the refactor/mssql-is-discovered-table-cache-helper branch from fdc9bc8 to 7c61d63 Compare July 1, 2026 14:06
Base automatically changed from fix/mssql-2-part-name-temp-table-filtering to master July 1, 2026 14:11
@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...n/src/datahub/ingestion/source/sql/mssql/source.py 93.33% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@maggiehays maggiehays added the needs-review Label for PRs that need review from a maintainer. label Jul 1, 2026
Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ingestion PR or Issue related to the ingestion of metadata needs-review Label for PRs that need review from a maintainer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants