-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ingestion/redshift): collapse lineage to permanent table #9704
feat(ingestion/redshift): collapse lineage to permanent table #9704
Conversation
…com:sid-acryl/datahub-fork into master+ing-473-redshift-temp-tables-lineage
…com:sid-acryl/datahub-fork into master+ing-473-redshift-temp-tables-lineage
…com:sid-acryl/datahub-fork into master+ing-473-redshift-temp-tables-lineage
start_time=datetime.now(), | ||
session_id="abc", | ||
create_command="CREATE TABLE #player_activity_temp", | ||
parsed_result=SqlParsingResult( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would've liked this test to be more broad. For example, we should be mocking the query results from the query that fetches temp tables and relying on the redshift lineage implementation to parse build TempTableRow
instances. That way, we would not have any hardcoded SqlParsingResult
objects in our tests and would ensure that we can actually fetch temp table definitions correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mock the query result for test_collapse_temp_lineage
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you didn't make similar changes to the rest of the tests?
@@ -1024,6 +1024,18 @@ def _is_dialect_instance( | |||
return False | |||
|
|||
|
|||
def get_query_type( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this method still used anywhere? if not, can we remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
|
||
query_vs_cursor_mocker = { | ||
( | ||
"-- DataHub Redshift Source temp table DDL query\n select\n *\n " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using """
would've been nicer to read than this, but it's fine
Redshift supports temporary table.
A permanent table can be derived from temporary table, we need to show upstream permanent table and it's table & column lineage.
To enable this feature set flag
resolve_temp_table_in_lineage
andinclude_table_rename_lineage
to true