feat(ingest): schema-aware SQL parsing for column-level lineage #8334
Conversation
There was a problem hiding this comment.
Haven't been able to go through this whole thing but looks good to me so far. I have a couple questions but nothing serious. I feel like there should be a cleaner way to do the -> urn conversion but haven't really thought about it, the "duplicate" classes _ColumnRef and ColumnRef just rubbing me the wrong way. Going to approve in case you need to merge, will finish review much later tonight
| @@ -285,6 +295,7 @@ def get_long_description(): | |||
| | bigquery_common | |||
| | { | |||
| *sqllineage_lib, | |||
| *sqlglot_lib, | |||
There was a problem hiding this comment.
Would be nice to get rid of bigquery beta if we can
| column_error: Optional[Exception] | ||
|
|
||
|
|
||
| class SqlParsingResult(BaseModel): |
There was a problem hiding this comment.
What's the difference between using pydantic here vs dataclasses? Mostly curious
| default_schema: Optional[str] = None, | ||
| ) -> SqlParsingResult: | ||
| # TODO: convert datahub platform names to sqlglot dialect | ||
| dialect = platform |
There was a problem hiding this comment.
Mostly unrelated to this PR, but we should make a platform enum
There was a problem hiding this comment.
Haven't been able to go through this whole thing but looks good to me so far. I have a couple questions but nothing serious. I feel like there should be a cleaner way to do the -> urn conversion but haven't really thought about it, the "duplicate" classes _ColumnRef and ColumnRef just rubbing me the wrong way. Going to approve in case you need to merge, will finish review much later tonight
|
@asikowitz going to merge here and fix in a follow up: #8382 |
| self._schema_cache: FileBackedDict[Optional[SchemaInfo]] = FileBackedDict( | ||
| shared_connection=shared_conn, | ||
| ) |
There was a problem hiding this comment.
Doesn't look like we ever close this, are we just relying on garbage collection for that? I could see that taking a while because references may be kept in memory by the lru_cache
There was a problem hiding this comment.
fixed in the follow up by explicitly calling self._make_schema_resolver.cache_clear()
Checklist