fix(glue): populate viewDefinition for Glue views - #33573
harshsoni2024 wants to merge 6 commits into
Conversation
- Add ViewOriginalText/ViewExpandedText to GlueTable; pydantic's extra="ignore" was silently dropping both off the boto3 response - New glue/utils.py: prefer ViewOriginalText, fall back to ViewExpandedText, decode the Presto/Trino base64 payload to its originalSql, and wrap a bare SELECT in CREATE VIEW so the lineage parser resolves a target table - Treat a comment-only candidate (/* Presto View */) as absent; warn only on a payload we were handed and could not read - Key the definition off Glue's VIRTUAL_VIEW, so Iceberg views keep theirs; ordinary, external and Iceberg tables are unchanged - Honour includeTables/includeViews, which Glue ignored entirely - Guard against a null StorageDescriptor/SerdeInfo on views Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
❌ 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
harshsoni2024
changed the title
harshsoni2024
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
View lineage can break for truncated names or SQL containing CREATE VIEW, and the PR description lacks the required linked issue.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Glue view-definition ingestion for Hive/Presto/Trino views, including decoding, fallback handling, and metadata safety.
Changes:
- Preserves view text in
GlueTable. - Populates
schemaDefinitionand honors table/view inclusion flags. - Adds extensive Glue view regression tests.
File summaries
| File | Description |
|---|---|
glue/utils.py |
Decodes and normalizes view definitions. |
glue/models.py |
Adds Glue view text fields. |
glue/metadata.py |
Applies definitions, filtering, and null-safe metadata handling. |
test_glue.py |
Tests parsing, typing, flags, and null metadata. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔵 Needs a closer look
View definitions can miss lineage targets for names exceeding normalization limits or SQL containing CREATE VIEW inside literals/comments.
Review details
Suppressed comments (2)
ingestion/src/metadata/ingestion/source/database/glue/metadata.py:361
- The request name is the normalized
table_namefromstandardize_table_name(which truncates Glue names to 128 characters), but the definition is built from rawtable.Name. For a view whose name is normalized,schemaDefinitiontargets a different identifier than the ingested entity, so view lineage/schema parsing cannot resolve the target; pass the same normalized name into the helper when constructing the definition.
is_view = table.TableType == "VIRTUAL_VIEW"
schema_definition = get_schema_definition(table, schema_name) if is_view else None
ingestion/src/metadata/ingestion/source/database/glue/utils.py:88
CREATE_VIEW_PATTERN.searchalso matches text inside a valid bare query, so a definition such asSELECT 'CREATE VIEW' AS markeris treated as an already-qualified DDL and is stored without theCREATE VIEW <schema>.<table> ASwrapper. That leaves the lineage parser without a target, defeating the wrapping done here; anchor the detection to the statement start (while handling leading comments) and add a regression case for literals/comments containingCREATE VIEW.
if CREATE_VIEW_PATTERN.search(definition):
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
- Anchor CREATE_VIEW_PATTERN to the head of the statement (past a leading comment), as saphana/metadata.py:70 does. An unanchored search read the CREATE VIEW inside a string literal as an existing header and left the SELECT unwrapped, costing the lineage parser its target table. - Build the statement from the stored table name, not table.Name: standardize_table_name truncates to 128 chars, so a longer Glue view named a target the catalog does not hold. - Cover the null-SerdeInfo guard, which StorageDescriptor=None never reached (StorageDetails() defaults SerdeInfo to a non-null value). - Reword a test docstring that read as a garden-path sentence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔵 Needs a closer look
Generated DDL can be invalid for reserved Glue identifiers, and the acknowledged include-flag breaking change lacks its release note.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
ingestion/src/metadata/ingestion/source/database/glue/utils.py:104
- This treats every ASCII word as safe to leave unquoted, but a valid Glue schema/table can still be a SQL reserved word such as
select. That would emitCREATE VIEW select.select AS ..., which is invalid for the SQL parser and loses the target the wrapper is meant to provide. Quote reserved words as well (or use the target dialect's identifier preparer) and add a regression case.
ingestion/src/metadata/ingestion/source/database/glue/metadata.py:295 - The PR description explicitly identifies honoring
includeTables/includeViewsas a breaking change and says it needs a release note, but the four-file diff adds none. A service that had either flag set to false will now stop emitting those entities and soft-delete them on the next run; document that migration impact before merging.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
CodeQL flagged the leading-comment group added in the previous commit. `(?:/\*.*?\*/\s*|--[^\n]*\n\s*)*` repeats a group whose body can also match the `*/` that ends it, so input like `"/*" + "*//*" * n` backtracks exponentially: 98 characters took 1s and doubled every 8 more, meaning a view definition of a couple hundred characters would hang the ingestion worker. `ViewOriginalText` comes from the catalog being ingested, so this is reachable from ordinary source data. Step over leading comments one at a time with `match(text, position)` instead of a repeated group. Now linear: 200KB in 6.9ms, with every header case behaving as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Reserved-word identifiers can generate invalid lineage DDL, and PR metadata validation is currently failing.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
ingestion/src/metadata/ingestion/source/database/glue/utils.py:112
_quotetreats every ASCII identifier as safe, but Glue names are not constrained by SQL reserved-word rules. A view/database namedselectwould emitCREATE VIEW select.select ..., where Athena/Trino parseselectas a keyword instead of an identifier, so the lineage parser cannot resolve the target. Quote reserved words too (or quote all identifiers with the target dialect's rules) and add a regression case.
ingestion/src/metadata/ingestion/source/database/glue/metadata.py:295- The PR description explicitly identifies honoring
includeTables/includeViewsas a breaking behavior that needs a release note, but the four-file change contains no release-note entry. Users who set either flag to false can have previously ingested entities soft-deleted on the next run without upgrade guidance; add the release note before merging.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
✅ Playwright Results — workflow succeededValidated commit ✅ 109 passed · ❌ 0 failed · 🟡 1 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) 42m 59s ⏱️ Max setup 4m 36s · max shard execution 12m 47s · max shard-job elapsed before upload 19m 20s · reporting 4s 🌐 236.26 requests/attempt · 1.80 app boots/UI scenario · 0.00% common-shard skew Optimization targets still in progress:
🟡 1 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 |
# Conflicts: # ingestion/tests/unit/topology/database/test_glue.py
There was a problem hiding this comment.
🟡 Changes recommended
Line-comment-only definitions are mishandled, and warning-level payload failures are counted in workflow summaries despite the stated contract.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
| # binascii.Error, UnicodeDecodeError and JSONDecodeError are all ValueError subclasses. | ||
| view_data = json.loads(base64.b64decode(encoded, validate=True).decode("utf-8")) | ||
| except ValueError as exc: | ||
| logger.warning("Could not read the Presto/Trino view payload for [%s]: %s", table_name, exc) |
- `_read_definition` stripped only `/* */`, so `-- generated by Athena` was wrapped into `CREATE VIEW ... AS -- generated by Athena`, whose whole body is commented out. - Both comment forms now come from one `COMMENT` fragment shared with the header check, which already knew about `--`. That drift between the two readers is what let a marker through. - Pin the run-summary contract: `StatusWarningHandler` (#27101) forwards every ingestion WARNING into the summary, so an undecodable payload is counted on purpose and a view Glue holds no text for is not. Tested against a real handler rather than asserted in the PR text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code Review ✅ Approved🟡 Medium risk Populates Glue view schema definitions by retaining view-text fields, decoding Presto/Trino payloads, and wrapping bare SQL as OptionsDisplay: compact → Counting what did not apply, without listing it. 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.
🟡 Changes recommended
Invalid base64 raises binascii.Error, which is not caught, causing malformed view payloads to fail ingestion instead of degrading gracefully.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
|
Describe your changes:
Fixes open-metadata/openmetadata-collate#5943
Glue
VIRTUAL_VIEWobjects were ingested asTableType.Viewbut with no definition, so the entity was only partially populated and could not support view-lineage parsing.The root cause is one thing that was never written:
GlueTablenever declaredViewOriginalText/ViewExpandedText. The model leaves pydantic'sextra="ignore"default in place, soTablePage(**page)silently dropped both keys off the boto3 response and the text never reachedyield_table— which in turn never passed a definition toCreateTableRequest.grep -r ViewOriginalText ingestion/returned zero hits before this PR.Note the field on
CreateTableRequestisschemaDefinition, notviewDefinition— the latter does not exist outside generated schema.Type of change:
The breaking part is the
includeTables/includeViewshandling — see Backward compatibility below. Both flags default totrue, so default configurations are unaffected.High-level design:
Two Glue view formats, and only one of them is SQL.
ViewOriginalTextViewExpandedTextSELECT ...)/* Presto View: <base64-JSON> *//* Presto View */— a marker, not SQLPassing the Athena blob straight through would satisfy "non-empty" while being unreadable in the UI and useless for lineage — and Glue + Athena is the dominant deployment. So the payload is base64-decoded and its
originalSqlextracted.Files
glue/models.py(+2) — declare the two fields so they survive parsing.glue/utils.py(new, 98 lines) —get_schema_definition(table, schema_name), a pure function of the model plus the schema name. Noself, no client, no topology context, so it is testable without constructing aGlueSource. Placed in a siblingutils.pyper the established connector layout (redshift/utils.py,hive/utils.py,postgres/utils.py) and the repo rule on keeping connector-specific logic in connector-specific files.glue/metadata.py(+20/-2) — one gated call, plus the include-flag checks and two null-safety guards.Contract, in order
ViewOriginalText, fall back toViewExpandedText. Never the reverse: the original is the user's SQL, the expanded text is Hive's rewrite. Implemented as a loop with a per-candidate usability test rather than anorchain, which is what makes the "Hive left the original empty" case deterministic./* (presto|trino) [materialized ]view: <payload> */is decoded. The colon is the discriminator — with it the comment carries a payload, without it it is the bare marker./* Presto View */,/* Trino View */and/* Presto Materialized View */, and guarantees a marker can never leak intoschemaDefinition.SELECTis wrapped intoCREATE VIEW <schema>.<table> AS ..., following the existing precedent inredshift/utils.py:494-502andtrino/metadata.py:317-321."zipcode-db"."my-view"). Glue allows hyphens in database names — the repo's own test fixture haszipcode-db— and the precedent's unquoted interpolation would emit unparseable SQL there.118146679784), not a SQL catalog, so it is deliberately left out of the wrapped name.logger.debug, while a payload we were handed and could not read (bad base64, not UTF-8, not JSON, nooriginalSql) is a singlelogger.warning. Deliberately notstatus.warning, which is counted into the run summary.Why the wrapping earns its place — measured through the repo's own
LineageParser:Without the wrap there is no target table, so no lineage edge.
Iceberg views. Glue types an Iceberg view as
VIRTUAL_VIEWand stampstable_type=ICEBERG, and the Iceberg branch wins the existing type ladder. Keying offTableType.Viewalone would have left exactly this shape without a definition, so the gate keys off Glue's ownVIRTUAL_VIEWinstead. The entity stays typedIceberg— that is unchanged — it just gains a definition. An Iceberg table isEXTERNAL_TABLEand cannot reach the branch.Alternatives rejected
ViewOriginalText— non-empty but stores/* Presto View: eyJvcmlnaW5hbFNxbCI6... */for every Athena view.ViewExpandedTextas a blind fallback — for Athena views that is the bare marker, which would silently poison the field.Backward compatibility
GlueSourceignoredincludeTables/includeViewsentirely, unlike the generic path (common_db_source.py:415) and unlike Delta Lake (deltalake/metadata.py:197-204), the other source that hand-builds its requests. This PR honours both. Both default totrue, so default configurations see no change; a service that explicitly set either tofalsehas been receiving those entities anyway and will now stop, with the entities soft-deleted on the next run. This needs a release note.Tests:
Use cases covered
TableType.Viewwith a decoded, human-readableCREATE VIEWdefinition.SELECTwrapped.TableType.Icebergand gains its definition.includeViews: falsedrops Presto, Hive and Iceberg views alike;includeTables: falsekeeps only views.Unit tests
I added unit tests for the new/changed logic.
Files updated:
ingestion/tests/unit/topology/database/test_glue.py(+297/-17)New classes:
TestGlueViewModel,TestGlueSchemaDefinition(14 parametrized cases),TestGlueSchemaDefinitionWarnings,TestGlueViewRequest,TestGlueIcebergView,TestGlueIncludeFlagsCoverage (
pytest --cov=metadata.ingestion.source.database.glue):glue/utils.py(new)glue/models.pyglue/metadata.pyEvery line changed by this PR is covered; the uncovered lines in
metadata.pyare pre-existing (thetableFilterPatternbranch and the per-table exception handler).Results: 54 passed in
test_glue.py; 1734 passed, 6 skipped acrosstests/unit/topology/database/.The tests are not tautological — reverting each source change was confirmed to fail the corresponding tests (the model field, the
schemaDefinition=line, the Iceberg gate, and the include flags), then restored.Backend integration tests
Ingestion integration tests
yield_tableagainst boto3-shaped fixtures instead.Playwright (UI) tests
Manual testing performed
Verified locally, without a live AWS account:
originalSql,catalog,schema,columns,owner,runAsInvoker), fed it through the fullTablePage(**raw_dict)boto3 path, and confirmed the decoded output:CREATE VIEW default.android_users AS SELECT *\nFROM\n cloudfront_logs\nWHERE (os = 'Android')CREATE VIEW "zipcode-db".hive_v AS SELECT a FROM tNone, no warning.LineageParser(Athena dialect) and confirmed the wrapped statement resolves a target table while the bareSELECTdoes not.ruff checkclean repo-wide; changed files formatted.UI screen recording / screenshots:
Not applicable — no UI code changes. The user-visible effect is that the existing Schema Definition tab, hidden by
isUndefined(tableDetails?.schemaDefinition), now renders for Glue views.Note on scope
View lineage for Glue is not enabled by this PR and is deliberately left out.
glueConnection.jsondeclares onlysupportsMetadataExtractionandsupportsDBTExtraction, andglue/service_spec.pyregisters nolineage_source_class, so a Glue lineage pipeline cannot be configured today. That needs a JSON Schema change plus aGlueLineageSource, and belongs in its own PR. PopulatingschemaDefinitionis the prerequisite that was missing —es_mixin.yield_es_view_defrequires both a view-ishtableTypeandexists: schemaDefinition, and this PR satisfies both.Checklist:
Fixesabove.TestGlueViewModel.test_view_text_survives_model_parsingis the regression test for the dropped-field root cause).The PR appears safe to merge; no outstanding correctness, security, or repository-rule issues were identified.
Summary
This PR enables complete ingestion of AWS Glue view definitions and aligns Glue discovery with configured table/view inclusion flags.
ViewOriginalTextandViewExpandedTextfrom Glue responses.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR A[AWS Glue table response] --> B[GlueTable model] B --> C{VIRTUAL_VIEW?} C -- No --> D[CreateTableRequest without schemaDefinition] C -- Yes --> E[Read original or expanded text] E --> F{Presto or Trino payload?} F -- Yes --> G[Decode originalSql] F -- No --> H[Use plain SQL] G --> I[Wrap as CREATE VIEW when needed] H --> I I --> J[CreateTableRequest with schemaDefinition]Reviews (5) · Last reviewed commit: "fix(glue): treat a line-comment-only vie..."