fix(glue): stop a custom databaseName dropping every schema - #31958
fix(glue): stop a custom databaseName dropping every schema#31958mohittilala wants to merge 5 commits into
Conversation
databaseName labels the OpenMetadata database, it does not name a Glue catalog. Schema discovery compared it against the Glue Catalog ID, so a custom name matched nothing and every schema was skipped while the run still reported Success. Skip that comparison when databaseName is set, since one name means one database and every visible Glue database is one of its schemas. Warn when that merges schemas from more than one catalog, so same-named schemas colliding is visible rather than silent. Also correct the connection schema description, which claimed the default database name is "default" while the code uses the Glue Catalog ID.
❌ 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
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes Glue ingestion behavior when a custom databaseName is configured, ensuring schemas are not incorrectly dropped due to an invalid CatalogId comparison, and improving user-facing configuration guidance.
Changes:
- Adjusted Glue schema discovery to only enforce CatalogId scoping when
databaseNameis not configured. - Added regression tests covering custom
databaseNameschema discovery and multi-catalog warning behavior. - Updated the Glue connection JSON schema tooltip/description to correctly explain
databaseNamesemantics and defaults.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| openmetadata-spec/src/main/resources/json/schema/entity/services/connections/database/glueConnection.json | Corrects databaseName description to match actual default (Glue Catalog ID) and clarifies it is a display name, not an ingestion selector. |
| ingestion/src/metadata/ingestion/source/database/glue/metadata.py | Fixes schema filtering logic for custom databaseName; adds a warning when multiple CatalogIds are merged under one OpenMetadata database name. |
| ingestion/tests/unit/topology/database/test_glue.py | Adds regression coverage for custom databaseName schema discovery and multi-catalog merge warning. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
✅ TypeScript Types Auto-UpdatedThe generated TypeScript types have been automatically updated based on JSON schema changes in this PR. |
| database_name = self.context.get().database | ||
| custom_database_name = self.service_connection.databaseName | ||
| catalog_ids_seen = set() | ||
| for page in self._get_glue_database_and_schemas() or []: | ||
| for schema in page.DatabaseList: | ||
| try: | ||
| if schema.CatalogId != database_name: | ||
| if not custom_database_name and schema.CatalogId != database_name: | ||
| continue | ||
| schema_fqn = fqn.build( |
_get_glue_tables called get_tables with DatabaseName only, so it read from the caller's default catalog. Now that a schema from another catalog can be yielded, its tables would come back empty or belong to a same-named database in the wrong catalog. Stash each schema's Catalog ID as it is yielded and pass it to get_tables. The default path is unaffected, since the database there already is the caller's Catalog ID.
Jest test CoverageUI tests summary
|
✅ Playwright Results — workflow succeededValidated commit ✅ 672 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) 59m 50s ⏱️ Max setup 6m 30s · max shard execution 20m 32s · max shard-job elapsed before upload 25m 37s · reporting 6s 🌐 223.69 requests/attempt · 2.66 app boots/UI scenario · 30.52% 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 |
|
GlueSchema.CatalogId is Optional, and adding it unguarded put None in the set used for the merge warning. Sorting that set then raised TypeError outside the per-schema try/except, so one database without a Catalog ID aborted schema discovery for the whole service. Track only non-empty Catalog IDs, reusing the guard the catalog map already applies.
Code Review ✅ ApprovedFixes Glue custom database name ingestion so schemas are no longer filtered out incorrectly, while preserving catalog identity during table lookup and adding multi-catalog warning coverage. No issues found. OptionsDisplay: 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 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 15 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
ingestion/src/metadata/ingestion/source/database/glue/metadata.py:224
- When
schema.CatalogIdis missing,schema_catalog_id_mapis left unchanged. Since this map is reused across schemas (and potentially across multiple Glue catalogs processed by the same source instance), a schema with a missing CatalogId but a name that was previously seen can inherit a stale CatalogId and cause_get_glue_tables()to paginate against the wrong catalog. Consider explicitly clearing any prior mapping for this schema name (e.g.,else: self.schema_catalog_id_map.pop(schema.Name, None)) when CatalogId is falsy.
if schema.Description:
self.schema_description_map[schema.Name] = Markdown(schema.Description)
if schema.CatalogId:
self.schema_catalog_id_map[schema.Name] = schema.CatalogId
catalog_ids_seen.add(schema.CatalogId)
| assert ["default", "foreign_schema"] == list(source.get_database_schema_names()) # noqa: SIM300 | ||
| assert len(source.status.warnings) == 1 | ||
| assert "more than one catalog" in str(source.status.warnings[0]) |
|
Fixes open-metadata/openmetadata-collate#5942
Summary
Setting the Glue
databaseNameproduced a successful ingestion with zero schemas and zero tables.get_database_namesyields the configureddatabaseNameverbatim, andget_database_schema_namesthen filtered withschema.CatalogId != database_name, comparing that label against the Glue Catalog ID (the AWS account ID). Unless the user happened to type their own account ID into the field, nothing matched, every schema was skipped, and the run still reported Success with no failures and no warnings.The comparison came from #26908, which is correct for the default path where the OpenMetadata database really is the Catalog ID. It was applied to the custom-name path too, where it can never hold.
What this changes
databaseNameis a label for the OpenMetadata database, not a selector for a Glue catalog, which is how the field behaves across connectors. So when it is set there is exactly one database and every visible Glue database is one of its schemas, and there is nothing to scope by Catalog ID. When it is unset nothing changes: the database is the Catalog ID and schemas from other catalogs still belong elsewhere.A previous attempt at this (#31820, closed unmerged) made the same call conditional, and review raised that it dropped catalog scoping altogether, so schemas with the same name in different catalogs could silently collide under one database. Rather than invent a scoping rule the field never promised, this reports the situation: if schemas from more than one Catalog ID actually land in the database, the run records a warning naming the catalogs and telling the user to clear the field to get one database per catalog.
Also corrects the
glueConnection.jsondescription for the field. It claimed the default isdefault, while the code uses the Glue Catalog ID andyaml.mdxalready says so. That description is the tooltip in the Add Service form, so the wrong version was the one users read.Validation
pytest -c ingestion/pyproject.toml ingestion/tests/unit/topology/database/test_glue.py -q, 11 passedpytest -c ingestion/pyproject.toml ingestion/tests/unit/topology/database/ -q, 1217 passedbasedpyrighton the changed source, 0 errors and 0 warnings, no new entries against the violation baselineruff checkandruff format --checkon the changed filesReproduced first by driving the real
get_database_namesintoget_database_schema_namessequence against the connector's own fixtures:Tests
Two regression tests, both of which fail on
main:databaseNamestill discovers every schema, with no failures and no warningsThe existing
test_database_schema_names_filters_other_catalogs_before_schema_filterfrom #26908 still passes, so the default path keeps its catalog scoping.Follow-ups, deliberately not in this PR
databaseFilterPatternis not applied to a customdatabaseNametoday. Making it apply would be consistent with other connectors but is a behaviour change beyond this fix.catalogIdconnection field, unlike Athena. That is the proper way to express "ingest this specific catalog" and is a feature rather than a bug fix.docs-omconnectors/database/glue/yaml.mdxshipsdatabaseName: database_namein its sample YAML, which used to be exactly the broken case. Harmless now, but the sample should say what the field does.Greptile Summary
The PR fixes Glue ingestion with a custom
databaseNameby retaining visible schemas and preserving each uniquely named schema’s catalog identity when retrieving tables.CatalogIdto Glue table pagination.databaseNameschema description and regenerates dependent UI models.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains; the previously reported loss of catalog identity before table retrieval is addressed by retaining each uniquely named schema’s Catalog ID and supplying it to Glue table pagination.
Important Files Changed
databaseNamelabels the OpenMetadata database and does not select a Glue catalog.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Enumerate Glue databases] --> B{Custom databaseName set?} B -- No --> C[Keep schemas matching current Catalog ID] B -- Yes --> D[Keep every visible schema] C --> E[Record schema CatalogId] D --> E E --> F[Create OpenMetadata schema] F --> G[Request Glue tables with DatabaseName and CatalogId] D --> H{Multiple Catalog IDs seen?} H -- Yes --> I[Record ingestion warning]Reviews (4): Last reviewed commit: "Merge branch 'main' into fix/11571-glue-..." | Re-trigger Greptile