refactor(ingest): Refactoring container creation to common place #6877
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Using auto-state handler in bigquery, sql-common and snowflake
github-actions
bot
added
the
ingestion
Fix presto on hive container generation
# Conflicts: # metadata-ingestion/src/datahub/ingestion/source/snowflake/snowflake_v2.py # metadata-ingestion/src/datahub/ingestion/source/sql/bigquery.py
hsheth2
reviewed
Dec 30, 2022
| platform=self.platform, | ||
| domain_config=self.config.domain, | ||
| domain_registry=self.domain_registry, | ||
| platform_instance=self.config.platform_instance, |
There was a problem hiding this comment.
platform, platform_instance, env, and probably even database should all already be in the database_container_key - can we simplify these calls?
hsheth2
reviewed
Jan 13, 2023
| dataset, | ||
| ["Dataset"], | ||
| database_container_key, | ||
| yield from gen_schema_containers( |
There was a problem hiding this comment.
Suggested change
| yield from gen_schema_containers( | |
| yield from gen_schema_container( |
# Conflicts: # metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/bigquery.py # metadata-ingestion/src/datahub/ingestion/source/snowflake/snowflake_v2.py # metadata-ingestion/src/datahub/ingestion/source/sql/presto_on_hive.py # metadata-ingestion/src/datahub/ingestion/source/sql/sql_common.py
# Conflicts: # metadata-ingestion/src/datahub/ingestion/source/sql/sql_common.py
# Conflicts: # metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/bigquery.py
hsheth2
requested changes
Jan 19, 2023
| if not env: | ||
| env = config.env | ||
|
|
||
| schema_container_key = ( |
There was a problem hiding this comment.
always overrides schema_container_key - probably not what we want
| assert platform | ||
|
|
||
| if not platform_instance: | ||
| platform_instance = config.platform_instance |
There was a problem hiding this comment.
ideally remove platform, platform_instance, and env options - pull from the key_container instead
hsheth2
reviewed
Jan 19, 2023
| @@ -1299,40 +1212,18 @@ def gen_database_containers( | |||
| else None, | |||
| ) | |||
|
|
|||
| self.stale_entity_removal_handler.add_entity_to_state( | |||
There was a problem hiding this comment.
make sure to remove these in other places too
hsheth2
approved these changes
Jan 20, 2023
| name=schema.name, | ||
| description=schema.comment, | ||
| schema=self.snowflake_identifier(schema.name), | ||
| database=self.snowflake_identifier(db_name), |
There was a problem hiding this comment.
still feels a bit annoying that we need to provide the name twice, but i guess we can fix in a later PR
ericyomi
pushed a commit
to ericyomi/datahub
that referenced
this pull request
Feb 8, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactoring container creation to common place
Using auto-state handler in bigquery, sql-common and snowflake
Checklist