feat(ingest/unity): GE Profiling #8951
Conversation
| @@ -329,6 +348,7 @@ def _should_ignore_column(self, sqlalchemy_type: sa.types.TypeEngine) -> bool: | |||
|
|
|||
| @_run_with_query_combiner | |||
| def _get_column_type(self, column_spec: _SingleColumnSpec, column: str) -> None: | |||
| # logger.info(f"{column} {self.dataset.columns} {self.dataset.columns[0]}") | |||
| class UnityCatalogAnalyzeProfilerConfig(UnityCatalogConfig): | ||
| method: Literal["analyze"] = "analyze" | ||
|
|
||
| # TODO: Reduce duplicate code with DataLakeProfilerConfig, GEProfilingConfig, SQLAlchemyConfig |
github-actions
bot
added
the
ingestion
maggiehays
added
the
hacktoberfest-accepted
| at=urlparse(self.workspace_url).netloc, | ||
| db=None, | ||
| uri_opts={ | ||
| "http_path": f"/sql/1.0/warehouses/{self.profiling.warehouse_id}" |
There was a problem hiding this comment.
This assumes use of SQL warehouse. Looks like this may take different formats.
http-path is the HTTP Path either to a Databricks SQL endpoint (e.g. /sql/1.0/endpoints/1234567890abcdef), or to a Databricks Runtime interactive cluster (e.g. /sql/protocolv1/o/1234567890123456/1234-123456-slid123).
There was a problem hiding this comment.
Hmm, good point. It's going to be a bit annoying to support both profilers :|
There was a problem hiding this comment.
Gonna hold off on this... want to get this in before I never get to it again
metadata-ingestion/src/datahub/ingestion/source/sql/sql_generic_profiler.py
Show resolved
Hide resolved
| @@ -70,7 +70,6 @@ def generate_profile_workunits( | |||
| self, | |||
| requests: List[TableProfilerRequest], | |||
| max_workers: int, | |||
There was a problem hiding this comment.
we can force these to be kwargs for clarity
| max_workers: int, | |
| *, max_workers: int, |
| default_factory=OperationConfig, | ||
| description="Experimental feature. To specify operation configs.", | ||
| ) | ||
| method: str |
There was a problem hiding this comment.
how does this show up in the docs? does this need a Field(description="docs")?
There was a problem hiding this comment.
Looks like it doesn't show up at all. I'll add a description but in general, our docs support for discriminated unions is not very good -- we don't show which type supports which options. I'll update example recipes to help here
| dataset_name, | ||
| size_in_bytes=table.size_in_bytes, | ||
| last_altered=table.last_altered, | ||
| rows_count=0, # Can't get row count ahead of time |
There was a problem hiding this comment.
shouldn't we pass None here?
| rows_count=0, # Can't get row count ahead of time | |
| rows_count=None, |
There was a problem hiding this comment.
Yeah, looks like we can do this now. We don't for some reason in sql_generic_profiler.py
| ): | ||
| # Profile only table level if dataset is filtered from profiling | ||
| # due to size limits alone | ||
| if self.is_dataset_eligible_for_profiling( |
There was a problem hiding this comment.
isn't this the same as the call two lines above?
There was a problem hiding this comment.
For this one, size_in_bytes is 0 lol
| self.ddl = None | ||
|
|
||
|
|
||
| class UnityCatalogGEProfiler(GenericProfiler): |
There was a problem hiding this comment.
the amount of duplicated code here is not ideal - we definitely will need to revisit the GE profiler at some point
There was a problem hiding this comment.
Yeah, ideally I'd merge this with Mayuri's changes but if I don't get this in now idk when it'll go in... don't really have time to iterate on this
Checklist