feat(ingest): automated term classification for snowflake #6376
Conversation
github-actions
bot
added
the
ingestion
mayurinehate
force-pushed
the
classification_engine
branch
from
50746cd
to
93a64ca
Compare
mayurinehate
force-pushed
the
classification_engine
branch
from
8f95490
to
37b6394
Compare
hsheth2
changed the title
| @@ -3,6 +3,7 @@ | |||
| from datetime import datetime | |||
| from typing import Dict, List, Optional | |||
|
|
|||
| import pandas as pd | |||
There was a problem hiding this comment.
nit: this should be a typing-only dep - see mypy's docs on TYPE_CHECKING
There was a problem hiding this comment.
also I believe this PR acryldata/datahub-classify#6 is going to remove the pandas dep from datahub-classify
There was a problem hiding this comment.
We are using pandas to read from snowflake cursor and translate to pandas dataframe in snowflake- https://docs.snowflake.com/en/user-guide/python-connector-pandas.html#migrating-to-pandas-dataframes. so this is required dependency for snowflake connector. I am not sure how converting this to typing-only dep will be useful.
There was a problem hiding this comment.
got it - in this case it's fine then, but I don't want to have a hard dep on pandas when we add classification to other sources
There was a problem hiding this comment.
Makes sense. I believe that would need refractor in classification_mixin as well. Maybe, we can replace sample_data input param with a callable function that takes column as input and returns list-like values structure as output ?
There was a problem hiding this comment.
Sure, although I suspect a dict {column name -> list[sample values]} is good enough for this
| # Ideally we do not want null values in sample data for a column. | ||
| # However that would require separate query per column and | ||
| # that would be expensive, hence not done. | ||
| def get_sample_values_for_table(self, conn, table_name, schema_name, db_name): |
There was a problem hiding this comment.
does this repeat work that the profiler does?
There was a problem hiding this comment.
Kind of, but not exactly. With current implementation(with GE), profile can get only limited number of sample values(upto 20).
hsheth2
changed the title
Checklist