Fixes 29997: Oracle ingestion get_pk_constraint NoSuchTableError for tables without a primary key#30206
Conversation
… without a PK (open-metadata#29997) Under SQLAlchemy >= 2.0, Oracle's get_pk_constraint raises NoSuchTableError for tables that have no primary key, instead of returning an empty dict as it did under SQLAlchemy 1.4. This aborts ingestion for every table lacking a PK. Wrap the call the same way the adjacent unique/foreign key constraint calls are already wrapped. Prepared with AI assistance (Claude Code, Anthropic), reviewed for correctness before submission.
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…mixin Per review feedback: the previous fix caught NoSuchTableError in the generic SqlColumnHandlerMixin, used by every SQL connector, before columns are even reflected — so for dialects where NoSuchTableError still means "table is genuinely missing", the error would be silently swallowed as "no PK" instead of surfacing. Move the fix into an Oracle-only override of _get_columns_with_constraints in OracleSource, since Oracle's get_multi_pk_constraint-based get_pk_constraint is the dialect confirmed to conflate "no PK" with "absent from result" under SQLAlchemy >= 2.0. Other dialects go through the untouched mixin, where a NoSuchTableError continues to propagate as a real "table not found" signal. Updated the regression test to exercise OracleSource's override directly.
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Per review feedback: the Oracle-only override of _get_columns_with_constraints was a near-verbatim ~75-line copy of SqlColumnHandlerMixin's implementation, differing only in the try/except around the PK fetch. Any future change to the shared unique/foreign-key handling would silently not reach Oracle. Split SqlColumnHandlerMixin._get_columns_with_constraints so the PK fetch is its own small method, _get_pk_constraint. OracleSource now overrides only that method; the rest of the constraint-assembly logic (unique/foreign keys, column-name cleanup) is inherited unchanged from the shared mixin via normal method dispatch, matching the existing super()-delegation pattern already used by BigQuery/Cockroach for _get_columns_with_constraints.
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
…oSuchTableError Per review feedback: catching NoSuchTableError from get_pk_constraint doesn't distinguish "table has no PK" from "table was dropped/renamed after listing but before this per-table reflection ran" — both raise the same exception. Silently swallowing it in the latter case would let yield_table emit stale zero-column metadata for a table that no longer exists, instead of reporting the failure. _get_pk_constraint now confirms via inspector.has_table() (a dedicated Oracle data-dictionary existence check, independent of the buggy bulk PK-lookup query) before treating NoSuchTableError as "no PK". If the table is confirmed gone, the error is re-raised and propagates up to yield_table's existing exception handler, which correctly reports it as a failed/missing table via StackTraceError. Added a test for the re-raise path alongside the existing no-PK path.
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Code Review ✅ Approved 1 resolved / 1 findingsResolves Oracle ingestion failures for tables lacking primary keys by wrapping ✅ 1 resolved✅ Quality: Oracle override duplicates ~75 lines of the base mixin
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
harshach
added
safe to test
|
🟡 Playwright Results — all passed (31 flaky)✅ 4540 passed · ❌ 0 failed · 🟡 31 flaky · ⏭️ 95 skipped
🟡 31 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
2 participants
Describe your changes:
Fixes #29997
I worked on this because the Oracle connector's metadata ingestion fails on every table that lacks a primary key, after the SQLAlchemy 1.4 → 2.0 migration in #26031.
Root cause: Under SQLAlchemy 2.0, Oracle's
Inspector.get_pk_constraint()is a thin wrapper over the new bulkget_multi_pk_constraint()API. That API only returns dict entries for tables that have at least one PK-constraint row; a table without a PK is simply absent, and the wrapper's_value_or_raisethen raisesNoSuchTableErrorfor "absent from dict" even though the table exists. Under SQLAlchemy 1.4, the same call returned an empty dict instead of raising._get_columns_with_constraintsinsql_column_handler.pycallsget_pk_constraintwith no exception handling (unlike the adjacentget_unique_constraints/get_foreign_keyscalls, which already tolerateNotImplementedError), so this regression aborts ingestion for every table without a PK.What changed and why: Wrapped the
get_pk_constraintcall in atry/except NoSuchTableError, treating it the same way as "no PK constraint" (empty dict), matching the existing pattern used for unique/foreign key constraints in the same function. By the time this call runs, the table itself is already known to exist (columns were already reflected), so aNoSuchTableErrorhere can only mean "no PK data," not "table missing."Type of change:
Tests:
Use cases covered
NoSuchTableErrorfromget_pk_constraintfor a table without a primary key) no longer aborts column/constraint extraction for that table; it now returns an empty PK column list, matching pre-regression behavior.Unit tests
ingestion/tests/unit/topology/database/test_oracle.py(test_get_columns_with_constraints_handles_no_pk_nosuchtableerror)Inspector.get_pk_constraintto raiseNoSuchTableErrorand assertsSqlColumnHandlerMixin._get_columns_with_constraintsreturns an empty PK column list instead of propagating the exception. Verified the test fails against the pre-fix code and passes after the fix.Manual testing performed
pytest ingestion/tests/unit/topology/database/test_oracle.py -v— all 15 tests pass (14 pre-existing + 1 new).ruff check/ruff format --checkon the touched files — clean.Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Prepared with AI assistance (Claude Code, Anthropic), reviewed for correctness before submission.
Greptile Summary
This PR prevents Oracle metadata ingestion from failing on tables without primary keys. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "fix: disambiguate missing table from no-..." | Re-trigger Greptile
Context used: