fix(oracle): use configured catalog for constraint reflection - #31676
Conversation
❌ 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 |
github-actions
Bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| if kw.get("oracle_resolve_synonyms", False): | ||
| rows = list( | ||
| self._get_synonyms( | ||
| connection, | ||
| schema, | ||
| [table_name], | ||
| dblink, | ||
| info_cache=kw.get("info_cache"), | ||
| ) | ||
| ) | ||
| if rows: | ||
| row = rows[0] | ||
| table_name = self.denormalize_name(row.table_name) | ||
| schema = self.denormalize_name(row.table_owner) | ||
| if row.db_link: |
There was a problem hiding this comment.
💡 Edge Case: Synonym-not-found path skips name denormalization
In _prepare_constraint_args, when oracle_resolve_synonyms=True but self._get_synonyms returns no rows, table_name/schema are left as the raw normalized (typically lowercase) values instead of being denormalized. The constraint query (WHERE ac.table_name = :table_name AND ac.owner = :owner) then compares against Oracle's uppercased catalog values and returns nothing, silently dropping PK/UQ/FK metadata for non-synonym tables. SQLAlchemy's original _prepare_reflection_args falls back to denormalize_name(table_name)/denormalize_name(schema or default_schema_name) in this case. Add an else branch that denormalizes when rows is empty (mirroring the non-synonym branch); note the existing get_columns has the same gap so this only bites when synonym resolution is enabled. The new tests only cover the synonym-found case, so this path is untested.
Denormalize the identifiers when synonym resolution is on but no synonym matches.:
if kw.get("oracle_resolve_synonyms", False):
rows = list(
self._get_synonyms(connection, schema, [table_name], dblink, info_cache=kw.get("info_cache"))
)
if rows:
row = rows[0]
table_name = self.denormalize_name(row.table_name)
schema = self.denormalize_name(row.table_owner)
if row.db_link:
dblink = row.db_link if row.db_link.startswith("@") else f"@{row.db_link}"
else:
table_name = self.denormalize_name(table_name)
schema = self.denormalize_name(schema or self.default_schema_name)
else:
table_name = self.denormalize_name(table_name)
schema = self.denormalize_name(schema or self.default_schema_name)
Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsRestores catalog-aware constraint reflection in the Oracle connector to prevent missing tables during ingestion when using DBA visibility views. Consider handling name denormalization when the synonym-not-found path is skipped in _prepare_constraint_args. 💡 Edge Case: Synonym-not-found path skips name denormalization📄 ingestion/src/metadata/ingestion/source/database/oracle/utils.py:422-436 In Denormalize the identifiers when synonym resolution is on but no synonym matches.🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if dblink and not dblink.startswith("@"): | ||
| dblink = f"@{dblink}" | ||
|
|
||
| if kw.get("oracle_resolve_synonyms", False): |
There was a problem hiding this comment.
When identifier-case preservation and synonym resolution are enabled, get_indexes_preserve_case forwards the setting as resolve_synonyms, but _prepare_constraint_args reads only oracle_resolve_synonyms. The primary-key lookup therefore queries the alias instead of its target, causing the PK-backed index to be reported as a regular index.
|
✅ Playwright Results — workflow succeededValidated commit ✅ 110 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 48m 36s ⏱️ Max setup 3m 12s · max shard execution 13m 21s · max shard-job elapsed before upload 19m 53s · reporting 5s 🌐 212.82 requests/attempt · 1.79 app boots/UI scenario · 0.00% common-shard skew Optimization targets still in progress:
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
Describe your changes:
Related to #29997
Oracle ingestion can enumerate a table through
DBA_TABLESwhile SQLAlchemy 2 reflects its constraints throughALL_CONSTRAINTS. For an ingestion account with catalog visibility but no direct table grants, that produces this per-table trace:DBA_TABLES: 1 table → ALL_CONSTRAINTS: no table → NoSuchTableError → table omitted from the runThis change binds the connector catalog-aware query to the active SQLAlchemy 2 public reflection methods:
get_pk_constraint,get_unique_constraints, andget_foreign_keys. The selectedDBAorALLprefix is therefore used consistently for table, column, and constraint discovery.The constraint query now returns
index_nameso unique constraints retain SQLAlchemy compatibleduplicates_indexmetadata.Type of change:
High-level design:
N/A — small connector bug fix.
Tests:
Use cases covered
DBA_*table discovery and constraint reflection use the same catalog.ALL_*table discovery and constraint reflection use the same catalog.Unit tests
ingestion/tests/unit/topology/database/test_oracle_constraint_reflection.py22 passedacross the Oracle connector tests and the new regression tests.Backend integration tests
Ingestion integration tests
DBA_TABLES=2andALL_TABLES=0; five columns, the primary key, a composite unique constraint, and a foreign key were returned.Playwright (UI) tests
Manual testing performed
22 passed.2661 files already formatted.0 errors.SqlColumnHandlerMixin._get_columns_with_constraintsagainst Oracle Free using catalog-only visibility and verified PK, unique, and FK metadata.UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>.Fixes #<issue-number>.Greptile Summary
The PR redirects Oracle primary-key, unique-constraint, and foreign-key reflection through the connector’s configured DBA or ALL catalog and preserves backing-index metadata.
Confidence Score: 4/5
The PR should not merge until synonym resolution remains enabled when preserve-case index reflection delegates to the new primary-key reflector.
The new primary-key implementation ignores the resolve_synonyms keyword used by the existing preserve-case index path, causing synonym-backed primary-key indexes to be emitted as ordinary indexes.
Files Needing Attention: ingestion/src/metadata/ingestion/source/database/oracle/utils.py
Important Files Changed
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile