fix(ingest): drop the sqlglot requirement from auto_resolve_lineage_urns - #19882
Conversation
_TableName's data core is a plain (database, db_schema, table) holder, but the module-level sqlglot import for its conversion methods forced every SchemaResolver consumer to install the sql-parser extra. - as_sqlglot_table moves to sqlglot_lineage as _table_name_as_sqlglot_table - from_sqlglot_table is deleted; sqlglot_lineage._table_name_from_sqlglot_table already superseded it and is what production calls - qualified() drops its unused dialect parameter
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Connector Tests ResultsAll connector tests passed for commit To skip connector tests, add the Autogenerated by the connector-tests CI pipeline. |
The module header credited test_module_import_does_not_pull_sqlglot with protecting the deferred-import pattern, but that guard passes even with the imports done eagerly. It now states the invariant it actually holds, and why the __future__ import is load-bearing: TYPE_CHECKING-only names appear in dataclass fields, which would be evaluated at class-body execution without it. The guard now also pins schema_resolver and schema_resolver_provider, the other two modules in the __init__ chokepoint.
PR SummaryOverview SQLGlot-specific table conversion is consolidated in Import-safety tests are tightened so importing Reviewed by Cursor Bugbot for commit 30185ba. Bugbot is set up for automated code reviews on this repo. Configure here. |
sgomezvillamor
left a comment
There was a problem hiding this comment.
Approving — no blockers, and I confirmed there's no breaking change.
I traced every removed or changed surface against origin/master rather than relying on the description:
_TableName.from_sqlglot_table— zero production callers on master (every hit is a test, plus its own recursion and two comments). So the documented non-equivalence forAnonymoustable expressions (table=''via_restore_mssql_temp_table_prefixvs. the function name) isn't reachable from any production path.as_sqlglot_table— one production caller, updated; the moved body is identical.qualified(dialect=…)— all four call sites use keyword args. Worth noting the downstream failure modes are loud either way: a keyword caller getsTypeError, a positional one hits a pydanticValidationErroronOptional[str]. And_TableNameis private with nothing re-exported fromdatahub/sql_parsing/__init__.py.- The config validator — strictly relaxes validation, so nothing that parsed before fails now. No
updating-datahub.mdentry needed. - The runtime premise —
schema_resolver.pyhas zerosqlglotoccurrences,schema_resolver_providerpulls only schema_resolver / graph filters / PerfTimer,sql_parsing_commonmentions it only in comments and dialect strings, andgraph/client.py's imports areTYPE_CHECKING+ function-local. The processor's runtime path uses onlyprovide_schema_resolver,SchemaResolver(...),resolve_urn()andmatch_columns_to_schema.
Nice side effect worth calling out: cube_lineage, dbt_common, kafka_connect/common and dremio_source all import _models, which is no longer an sqlglot carrier for any of them.
Both test deletions look right to me — one covered code that no longer exists, and test_qualified_preserves_temp_prefix was genuinely vacuous since qualified() copies self.table verbatim.
Two non-blocking nits inline. One thing to watch before merge: connector-tests failed on 32fb216 and was still in progress on head when I looked.
Generated by Claude Code
| "assert 'sqlglot' not in sys.modules, 'sqlglot imported at module load'" | ||
| "assert 'sqlglot' not in sys.modules, 'sqlglot imported at module load'; " | ||
| "import datahub.sql_parsing.schema_resolver; " | ||
| "assert 'sqlglot' not in sys.modules, 'schema_resolver pulled in sqlglot'; " |
There was a problem hiding this comment.
Nit: this guard lives somewhere a sql_parsing/ developer won't look.
Extending the assertion here is the right call — it's the invariant the whole PR rests on. But the test now constrains datahub.sql_parsing.schema_resolver while sitting under tests/unit/workunit_processors/. Someone adding a module-level import sqlglot to schema_resolver.py has no reason to look in this file, and CI will point them at a workunit-processor test for a change they made in sql_parsing/.
Either a mirrored guard under tests/unit/sql_parsing/, or — cheaper — a one-line note at the top of schema_resolver.py:
# Must stay sqlglot-free: auto_resolve_lineage_urns relies on this module being
# importable without the [sql-parser] extra. Guarded by
# tests/unit/workunit_processors/test_auto_resolve_lineage_urns.py::test_module_import_does_not_pull_sqlglotNon-blocking.
Generated by Claude Code
What changed
auto_resolve_lineage_urnsfailed config parse whensqlglotwas not installed:The processor never parses SQL. It uses exactly two things from
datahub.sql_parsing:SchemaResolver.resolve_urn(urn)— an exact-URN cache lookup plus graph fetchmatch_columns_to_schema()— a lowercase column mapNeither needs sqlglot. The requirement came from an import chain:
schema_resolver.pyimports
_TableNamefromsql_parsing/_models.py, which did a module-levelimport sqlglotfor three conversion methods._TableName's data core is just(database, db_schema, table, parts)and none of that needs sqlglot._models.pyis now sqlglot-free, so importingschema_resolverno longer pullssqlglot and the config-parse guard is unnecessary.
The feature becomes usable on any connector, not only those whose extra bundles
sql-parser. No behaviour change when sqlglot is installed.Code removed
_TableName.from_sqlglot_table— dead.sqlglot_lineage.pyalready has_table_name_from_sqlglot_table, which is what every production path calls; theclassmethod had no callers outside tests. The two duplicated the same SemanticView and
Dot traversal, with a comment stating they were kept in sync by hand.
The surviving one handles two cases the deleted copy got wrong: it restores MSSQL
temp-table prefixes (
#,##) and splits SnowflakeIDENTIFIER('db.sch.tbl')into itsparts. It is not a strict superset — for
Anonymoustable expressions (table-valuedfunctions, BigQuery
EXTERNAL_QUERY) it yieldstable=''where the deleted methodyielded the function name. That is pre-existing production behaviour, since the deleted
method had no production callers, so this PR does not change it.
The 19 test call sites moved across with no assertion changes.
_TableName.qualified(dialect=...)— thedialectparameter was never read in themethod body. Dropped, along with it at the four call sites.
AutoResolveLineageUrnsConfig._require_sql_parser_when_enabled— the validator thatraised when sqlglot was missing, plus the matching sentence in the
enabledfielddescription and the caveat in
lineage_urn_casing.md.Code moved
_TableName.as_sqlglot_table→sqlglot_lineage._table_name_as_sqlglot_table(),which is where its single caller lives.
Tests
test_module_import_does_not_pull_sqlglotnow also asserts that importingdatahub.sql_parsing.schema_resolverleaves sqlglot out ofsys.modules. This is theguard for the invariant the change relies on; previously nothing covered it, because
the processor defers its
schema_resolverimport and the test passed either way.Two tests removed:
test_config_requires_sql_parser_only_when_enabledasserted the deleted validator.test_qualified_preserves_temp_prefixwas vacuous —qualified()copiesself.tableverbatim and never inspected the dialect, so the#it checked for wassimply its own input.
Verified with
tests/unit/sql_parsing,tests/unit/dremio,tests/unit/cube,tests/unit/dbt,tests/unit/kafka_connect,tests/unit/sdk_v2/test_dataset.pyandtests/unit/workunit_processors(1901 passed), plus./gradlew :metadata-ingestion:lint(ruff and mypy clean).
Checklist
🤖 Generated with Claude Code