fix(ingestion/oracle): Improved foreign key handling #11867

acrylJonny · 2024-11-15T12:33:28Z

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

treff7es · 2025-01-06T13:33:02Z

metadata-ingestion/src/datahub/ingestion/source/sql/oracle.py

+            "\nac.constraint_name,"
+            "\nac.constraint_type,"
+            "\nacc.column_name AS local_column,"
+            "\nNULL AS remote_table,"


Why do you set these to NULL?
Aren't these NULL anyway if we omit to select R constraints? (if I guess right and R means remote tables`.

That is true, but this does end up being a little more efficient splitting this out into 2 UNION'ed queries on the Oracle side.

treff7es · 2025-01-06T13:34:27Z

metadata-ingestion/src/datahub/ingestion/source/sql/oracle.py

+            "\nON ac.owner = acc.owner"
+            "\nAND ac.constraint_name = acc.constraint_name"
+            "\nAND ac.table_name = acc.table_name"
+            "\nWHERE ac.table_name = :table_name"
            "\nAND ac.constraint_type IN ('R','P', 'U', 'C')"


Shouldn't R be omitted from this as it will be UNION-ed anyway. This way it will generate duplicate rows.

fair point - I'll adjust

treff7es · 2025-01-06T13:35:04Z

metadata-ingestion/src/datahub/ingestion/source/sql/oracle.py

        )

-        text = text % {"dblink": dblink}
+        if schema is not None:


how do schema and owner come together?

https://docs.oracle.com/cd/B16351_01/doc/server.102/b14196/schema001.htm#:~:text=A%20schema%20is%20owned%20by,as%20a%20view%20or%20synonym.

treff7es · 2025-01-06T13:36:55Z

metadata-ingestion/src/datahub/ingestion/source/sql/oracle.py

@@ -552,8 +561,8 @@ def get_view_definition(
        text = "SELECT text FROM dba_views WHERE view_name=:view_name"

        if schema is not None:
-            text += " AND owner = :schema"
-            params["schema"] = schema
+            params["owner"] = schema


Why is schema set as an owner?

In Oracle's DBA_VIEWS, the OWNER column represents the schema name that owns the view.

hsheth2 · 2025-01-06T19:43:09Z

metadata-ingestion/src/datahub/ingestion/source/sql/oracle.py

+                    )  # local_column
+                    pkeys.append(col_name)
+        except Exception as e:
+            logger.error(


should be using our structured reporting - see https://acryldata.notion.site/Error-reporting-in-ingestion-5343cc6ea0c84633b38070d1a409c569?pvs=4

as a reporter hasn't been implemented at all in the Oracle connector I imagine that we should do that, right? There are a number of other areas that are currently logged but not reported.

The oracle source uses our SQL common source internally, which does do this reporting in most places.

Overall, I'd like to opportunistically move towards structured logging whenever we make tweaks to connectors

The reporter's errors/warnings have now been added.

codecov · 2025-01-10T11:00:36Z

Codecov Report

Attention: Patch coverage is 65.21739% with 8 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...gestion/src/datahub/ingestion/source/sql/oracle.py	65.21%	8 Missing ⚠️

Files with missing lines	Coverage Δ
...gestion/src/datahub/ingestion/source/sql/oracle.py	`75.00% <65.21%> (-2.10%)`	⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efc5d31...f2ad6a1. Read the comment docs.

acrylJonny added 2 commits November 12, 2024 19:07

Update oracle.py

9cc624f

Update oracle.py

b7e85bf

acrylJonny marked this pull request as draft November 15, 2024 12:34

Merge branch 'master' into oracle-updates

daa506b

github-actions bot added ingestion PR or Issue related to the ingestion of metadata community-contribution PR or Issue raised by member(s) of DataHub Community labels Nov 15, 2024

acrylJonny added 3 commits November 15, 2024 12:43

Update oracle.py

f4a2e01

Update oracle.py

baae9a2

Update oracle.py

59c9b05

vercel bot deployed to Preview November 15, 2024 13:13 View deployment

linting

630a889

vercel bot deployed to Preview November 15, 2024 13:45 View deployment

Update oracle.py

9f49414

vercel bot deployed to Preview November 15, 2024 15:46 View deployment

acrylJonny marked this pull request as ready for review November 15, 2024 17:02

Merge branch 'master' into oracle-updates

d8fb4c8

vercel bot deployed to Preview November 15, 2024 17:24 View deployment

anshbansal removed the community-contribution PR or Issue raised by member(s) of DataHub Community label Dec 4, 2024

datahub-cyborg bot added the pending-submitter-response Issue/request has been reviewed but requires a response from the submitter label Dec 4, 2024

Merge branch 'master' into oracle-updates

17c01b4

vercel bot deployed to Preview December 10, 2024 18:04 View deployment

Merge branch 'master' into oracle-updates

f82529f

vercel bot deployed to Preview January 6, 2025 10:00 View deployment

treff7es reviewed Jan 6, 2025

View reviewed changes

acrylJonny and others added 2 commits January 6, 2025 16:08

address comments and ensure consistent ac.owner input

db56578

Merge branch 'master' into oracle-updates

d385e95

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 Jan 6, 2025

treff7es approved these changes Jan 6, 2025

View reviewed changes

datahub-cyborg bot removed the needs-review Label for PRs that need review from a maintainer. label Jan 6, 2025

datahub-cyborg bot added the merge-pending-ci A PR that has passed review and should be merged once CI is green. label Jan 6, 2025

acrylJonny and others added 2 commits January 6, 2025 16:43

removing casts to varchar2 as param & fixing small alias issue

f48ea6b

Merge branch 'master' into oracle-updates

bec592d

vercel bot deployed to Preview January 6, 2025 17:09 View deployment

hsheth2 reviewed Jan 6, 2025

View reviewed changes

acrylJonny and others added 3 commits January 9, 2025 11:53

Update oracle.py

e91e5cb

Update oracle.py

1fdeca3

Merge branch 'master' into oracle-updates

f2ad6a1

vercel bot deployed to Preview January 10, 2025 11:19 View deployment

acrylJonny and others added 2 commits January 16, 2025 23:00

add tests

5e2539e

Merge branch 'master' into oracle-updates

fcae9f3

vercel bot deployed to Preview January 16, 2025 23:26 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ingestion/oracle): Improved foreign key handling #11867

fix(ingestion/oracle): Improved foreign key handling #11867

acrylJonny commented Nov 15, 2024

treff7es Jan 6, 2025

acrylJonny Jan 6, 2025

treff7es Jan 6, 2025

acrylJonny Jan 6, 2025

treff7es Jan 6, 2025

acrylJonny Jan 6, 2025

treff7es Jan 6, 2025

acrylJonny Jan 6, 2025

hsheth2 Jan 6, 2025

acrylJonny Jan 8, 2025

hsheth2 Jan 8, 2025

acrylJonny Jan 10, 2025

codecov bot commented Jan 10, 2025 •

edited

Loading

fix(ingestion/oracle): Improved foreign key handling #11867

Are you sure you want to change the base?

fix(ingestion/oracle): Improved foreign key handling #11867

Conversation

acrylJonny commented Nov 15, 2024

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

codecov bot commented Jan 10, 2025 •

edited

Loading