fix(ingest): replace sqllineage/sqlparse with our SQL parser #12020

sagar-salvi-apptware · 2024-12-03T17:21:02Z

We currently use sqllineage for mode/superset lineage, despite having a better SQL parser implemented.
We currently depend on sqllineage==1.3.8, which transitively depends on sqlparse==0.4.4
sqlparse==0.4.4 has a high sev vulnerability (see https://security.snyk.io/package/pip/sqlparse/0.4.4), and the first fixed version is 0.5.x
The first sqllineage version compatible with sqllparse 0.5.x is sqllineage 1.5.x. However, that depends on sqlalchemy>2, which many of our dependencies are incompatible with.
The easiest fix was to just move everything to use our sqlglot parser instead of upgrading our dep.

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

mayurinehate

Looks reasonable.
Awaiting response on comments and few changes around reporting.

metadata-ingestion/src/datahub/ingestion/source/mode.py

metadata-ingestion/src/datahub/ingestion/source/redash.py

metadata-ingestion/src/datahub/ingestion/source/unity/usage.py

metadata-ingestion/tests/unit/test_redash_source.py

metadata-ingestion/tests/unit/utilities/test_utilities.py

hsheth2

Will also need updates to gx-plugin

hsheth2 · 2024-12-05T16:21:07Z

metadata-ingestion/tests/unit/utilities/test_utilities.py

-        "column-timestamp",
-    ]
-    assert sorted(SqlLineageSQLParser(sql_query).get_tables()) == expected_tables
-    assert sorted(SqlLineageSQLParser(sql_query).get_columns()) == expected_columns


let's keep these tests around, and make it test with the new parser

see

datahub/metadata-ingestion/tests/unit/bigquery/test_bigquery_sql_lineage.py

Lines 7 to 8 in b814469

class BigQuerySQLParser:

for an example

Hi @hsheth2,

I've updated the tests to align with SQLGlot, and I noticed some changes in the outputs. Could you please verify them?
CC. @mayurinehate

docs/how/updating-datahub.md

metadata-ingestion-modules/gx-plugin/setup.py

hsheth2 · 2024-12-06T18:50:59Z

metadata-ingestion/docs/sources/redash/redash.md

@@ -1,5 +0,0 @@
-Note! The integration can use an SQL parser to try to parse the tables the chart depends on. This parsing is disabled by default, 
-but can be enabled by setting `parse_table_names_from_sql: true`.  The default parser is based on the [`sqllineage`](https://pypi.org/project/sqllineage/) package.


we still require them to set parse_table_names_from_sql right?

let's update the docs here instead of removing them entirely

Yes we need parse_table_names_from_sql, Added again

metadata-ingestion/src/datahub/ingestion/source/unity/usage.py

hsheth2 · 2024-12-06T18:53:20Z

metadata-ingestion/src/datahub/ingestion/source/unity/usage.py

        try:
-            runner = LineageRunner(query)
+            sql_parser_in_tables = create_lineage_sql_parsed_result(


we should probably be setting the default db / schema when calling create_lineage_sql_parsed_result

hi @hsheth2 , We don’t need it here since we only require table names. In the previous implementation as well, we didn’t include the database/schema.

metadata-ingestion/tests/unit/test_redash_source.py

metadata-ingestion/tests/unit/utilities/test_utilities.py

hsheth2 · 2024-12-06T18:58:21Z

metadata-ingestion/tests/unit/utilities/test_utilities.py

    tables_list.sort()
-    assert tables_list == ["my_view.SQL_TABLE_NAME"]


this change in particular seems problematic - can you look at the git history to understand why this test was added, and what we need to do to actually maintain most of the previous functionality? we'll likely need to tweak one of our sources

hi @hsheth2
It was added for looker token, but right now its not being used anymore
so technically this Test-Case is invalid
Let me your thought on this ?

CC. @mayurinehate

e95446b

hsheth2 · 2024-12-06T19:00:26Z

metadata-ingestion/tests/unit/utilities/test_utilities.py

+                ans.add(col_ref.column)
+        return list(ans)
+
+    def get_downstream_columns(self) -> List[str]:


it seems like the new get_downstream_columns is similar in functionality to the original get_columns? or am I mistaken on that?

overall I'm a bit confused why you added that extra method, when we previously didn't have anything like it?

Hi @hsheth2 ,

I added the get_downstream_columns method to verify what we’re getting in downstream columns since our new parser provides both upstream and downstream column lineage, which wasn’t possible with sqllineage.

Additionally, I noticed that the get_columns method from the sqllineage parser isn’t used anywhere in the source code—it’s only referenced in test cases.

I’ve now removed the get_downstream_columns method.

Stale review

github-actions bot added the ingestion PR or Issue related to the ingestion of metadata label Dec 3, 2024

datahub-cyborg bot added the needs-review Label for PRs that need review from a maintainer. label Dec 3, 2024

vercel bot deployed to Preview December 3, 2024 18:11 View deployment

hsheth2 requested a review from mayurinehate December 4, 2024 04:03

sagar-salvi-apptware added 2 commits December 4, 2024 12:18

fix: Remove sqlparse and sqllineage deps

02e7b10

fix: remove sqllineage_lib from redshift and tableau

8f9db11

sagar-salvi-apptware force-pushed the fix/ING-722-remove-sqlparse-linege-deps branch from 029ed63 to 8f9db11 Compare December 4, 2024 06:48

vercel bot deployed to Preview December 4, 2024 07:20 View deployment

fix: fixed unit-tests

4ee8c5a

vercel bot deployed to Preview December 4, 2024 13:10 View deployment

test: fix test failure

74c106d

vercel bot deployed to Preview December 4, 2024 18:49 View deployment

mayurinehate previously requested changes Dec 5, 2024

View reviewed changes

datahub-cyborg bot added pending-submitter-response Issue/request has been reviewed but requires a response from the submitter and removed needs-review Label for PRs that need review from a maintainer. labels Dec 5, 2024

fix: PR comments

84de369

datahub-cyborg bot added needs-review Label for PRs that need review from a maintainer. and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Dec 5, 2024

vercel bot deployed to Preview December 5, 2024 13:48 View deployment

hsheth2 requested changes Dec 5, 2024

View reviewed changes

datahub-cyborg bot added pending-submitter-response Issue/request has been reviewed but requires a response from the submitter and removed needs-review Label for PRs that need review from a maintainer. labels Dec 5, 2024

fix: removed deps gx-plugin

1aa0a24

datahub-cyborg bot added needs-review Label for PRs that need review from a maintainer. and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Dec 6, 2024

vercel bot deployed to Preview December 6, 2024 07:07 View deployment

mayurinehate reviewed Dec 6, 2024

View reviewed changes

docs/how/updating-datahub.md Outdated Show resolved Hide resolved

datahub-cyborg bot added pending-submitter-response Issue/request has been reviewed but requires a response from the submitter and removed needs-review Label for PRs that need review from a maintainer. labels Dec 6, 2024

test: added tests for sqlglot

461025b

fix: PR comments

4427621

datahub-cyborg bot added needs-review Label for PRs that need review from a maintainer. and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Dec 6, 2024

vercel bot deployed to Preview December 6, 2024 10:32 View deployment

hsheth2 requested changes Dec 6, 2024

View reviewed changes

datahub-cyborg bot added pending-submitter-response Issue/request has been reviewed but requires a response from the submitter and removed needs-review Label for PRs that need review from a maintainer. labels Dec 6, 2024

sagar-salvi-apptware force-pushed the fix/ING-722-remove-sqlparse-linege-deps branch from 103eb28 to c1d7596 Compare December 9, 2024 10:10

datahub-cyborg bot added needs-review Label for PRs that need review from a maintainer. and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Dec 9, 2024

fix: PR comments

135561c

sagar-salvi-apptware force-pushed the fix/ING-722-remove-sqlparse-linege-deps branch from c1d7596 to 135561c Compare December 9, 2024 10:12

vercel bot deployed to Preview December 9, 2024 10:46 View deployment

hsheth2 approved these changes Dec 9, 2024

View reviewed changes

datahub-cyborg bot added merge-pending-ci A PR that has passed review and should be merged once CI is green. and removed needs-review Label for PRs that need review from a maintainer. labels Dec 9, 2024

hsheth2 approved these changes Dec 9, 2024

View reviewed changes

Merge branch 'master' into fix/ING-722-remove-sqlparse-linege-deps

70d2606

vercel bot deployed to Preview December 10, 2024 05:27 View deployment

hsheth2 changed the title ~~fix: Remove sqlparse and sqllineage deps~~ fix(ingest): replace sqllineage/sqlparse with our SQL parser Dec 10, 2024

hsheth2 merged commit 57b12bd into datahub-project:master Dec 10, 2024
73 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ingest): replace sqllineage/sqlparse with our SQL parser #12020

fix(ingest): replace sqllineage/sqlparse with our SQL parser #12020

sagar-salvi-apptware commented Dec 3, 2024 •

edited by hsheth2

Loading

mayurinehate left a comment

hsheth2 left a comment

hsheth2 Dec 5, 2024

sagar-salvi-apptware Dec 6, 2024 •

edited

Loading

hsheth2 Dec 6, 2024

sagar-salvi-apptware Dec 9, 2024 •

edited

Loading

hsheth2 Dec 6, 2024

sagar-salvi-apptware Dec 9, 2024

hsheth2 Dec 6, 2024

sagar-salvi-apptware Dec 9, 2024 •

edited

Loading

hsheth2 Dec 6, 2024

sagar-salvi-apptware Dec 9, 2024 •

edited

Loading

		@@ -1,5 +0,0 @@
		Note! The integration can use an SQL parser to try to parse the tables the chart depends on. This parsing is disabled by default,
		but can be enabled by setting `parse_table_names_from_sql: true`. The default parser is based on the [`sqllineage`](https://pypi.org/project/sqllineage/) package.

		tables_list.sort()
		assert tables_list == ["my_view.SQL_TABLE_NAME"]

fix(ingest): replace sqllineage/sqlparse with our SQL parser #12020

fix(ingest): replace sqllineage/sqlparse with our SQL parser #12020

Conversation

sagar-salvi-apptware commented Dec 3, 2024 • edited by hsheth2 Loading

Checklist

mayurinehate left a comment

Choose a reason for hiding this comment

hsheth2 left a comment

Choose a reason for hiding this comment

hsheth2 Dec 5, 2024

Choose a reason for hiding this comment

sagar-salvi-apptware Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

hsheth2 Dec 6, 2024

Choose a reason for hiding this comment

sagar-salvi-apptware Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

hsheth2 Dec 6, 2024

Choose a reason for hiding this comment

sagar-salvi-apptware Dec 9, 2024

Choose a reason for hiding this comment

hsheth2 Dec 6, 2024

Choose a reason for hiding this comment

sagar-salvi-apptware Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

hsheth2 Dec 6, 2024

Choose a reason for hiding this comment

sagar-salvi-apptware Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

sagar-salvi-apptware commented Dec 3, 2024 •

edited by hsheth2

Loading

sagar-salvi-apptware Dec 6, 2024 •

edited

Loading

sagar-salvi-apptware Dec 9, 2024 •

edited

Loading

sagar-salvi-apptware Dec 9, 2024 •

edited

Loading

sagar-salvi-apptware Dec 9, 2024 •

edited

Loading