Fixes #31515: include materialized views in PostgreSQL and Greenplum connectors - #31549
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 |
|
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! |
IceS2
left a comment
There was a problem hiding this comment.
Hey @zerafachris,
Thanks for working on this.
There is one important issue with the current approach: POSTGRES_GET_TABLE_NAMES and GREENPLUM_GET_TABLE_NAMES are called from query_table_names_and_types(), which only runs when includeTables=true. Views are controlled separately through query_view_names_and_types() and includeViews.
This creates inverted behavior:
includeTables=false,includeViews=true→ the materialized view is still missing.includeTables=true,includeViews=false→ the materialized view is ingested even though views are disabled.
Could you move materialized-view discovery to the view path instead?
- Restore the PostgreSQL and Greenplum table-discovery queries.
- Override
query_view_names_and_types()to combineInspector.get_view_names()as View withInspector.get_materialized_view_names()as MaterializedView.
The current tests only check that a mapping or SQL string contains 'm', so they can pass while the configuration behavior is wrong. Please add regression coverage for these two cases:
- includeTables=false, includeViews=true → materialized view is emitted as MaterializedView.
- includeTables=true, includeViews=false → materialized view is not emitted.
PostgreSQL already has a testcontainer-based integration suite under ingestion/tests/integration/postgres/, so the first case can be covered against a real materialized view.
For Greenplum, a focused behavior test around query_view_names_and_types() would still be valuable.
|
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! |
3 similar comments
|
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! |
|
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! |
|
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! |
|
Thanks for the detailed review @IceS2! You're absolutely right — the table-path approach created inverted behaviour. I've reworked the fix across 5 commits on this branch: Core change (moved to view path):
Tests added:
The existing Prepared with AI assistance (Claude Code, Anthropic), reviewed for correctness before submission. |
|
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! |
…iew_names_and_types If get_materialized_view_names() raises (e.g. older Greenplum/Postgres server without matview support, or a permission error), catch the exception, log a warning, and return an empty matview list rather than propagating the error and silently dropping all regular views for the schema. Addresses Gitar review suggestion on PR open-metadata#31549.
…view_names_and_types If get_materialized_view_names() raises (e.g. Greenplum server version that does not support matviews, or a permission error), catch the exception, log a warning, and return an empty matview list rather than propagating the error and silently dropping all regular views for the schema. Addresses Gitar review suggestion on PR open-metadata#31549.
|
Good catch — addressed in the latest two commits. Both |
|
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! |
1 similar comment
|
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! |
|
Thanks @IceS2 — great catch on the includeTables/includeViews inversion. Address pushed: What changed:
Materialized views are now only discovered when |
|
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! |
2 similar comments
|
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! |
|
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! |
|
Applied |
|
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! |
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
|
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! |
…m connectors Materialized views are now discovered through query_view_names_and_types() (controlled by includeViews) instead of the table path (includeTables). This fixes the inverted config behavior flagged in review: - includeViews=false, includeTables=true → matviews were wrongly ingested - includeViews=true, includeTables=false → matviews were silently missing Changes: - Add PgMatviewMixin to common_pg_mappings.py: overrides query_view_names_and_types to chain get_view_names() as View and get_materialized_view_names() as MaterializedView; guards against inspectors that lack get_materialized_view_names - PostgresSource and GreenplumSource inherit PgMatviewMixin - Restore POSTGRES_GET_TABLE_NAMES and GREENPLUM_GET_TABLE_NAMES to table-only filters (relkind ≠ 'm') - Add behavioral unit tests for both connectors: - includeViews=true → matview emitted as MaterializedView - includeViews=false → matview NOT emitted (view path not called) - matview is absent from the table path regardless of includeTables - Add integration test fixture in test_metadata.py for a real materialized view Fixes: open-metadata#31515 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
df318d3 to
dcb5fa5
Compare
|
Hi @IceS2 — thanks for the detailed review. All three points addressed in the rebased commit: Inverted config behavior fixed: Materialized views are now discovered in Behavioral regression tests added:
Integration test: PgMatviewMixin guards against inspectors that lack Branch rebased cleanly onto current main (squashed to one commit to eliminate the noisy merge-into-branch history). Prepared with AI assistance (Claude Code, Anthropic), reviewed for correctness before submission. |
|
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! |
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
…ources The rebase onto main replayed this branch's pre-open-metadata#31643 copies of postgres/metadata.py and greenplum/metadata.py, which reverted the typing modernisation main had already applied and reintroduced the `# noqa: UP035`, `# noqa: UP045` and `# noqa: UP006` suppressions that came with it. `ingestion/scripts/check_ruff_suppressions.py`, wired into `py_format_check` by open-metadata#31643, forbids suppressing UP006/UP007/UP035/UP045, so py-checkstyle failed with 10 violations across the two files. Dropping the suppressions and letting `ruff check --fix` resolve the underlying violations restores both files to main's content: - `typing.Iterable` -> `collections.abc.Iterable` - `Optional[X]` -> `X | None` - `Tuple[...]` -> `tuple[...]` Only type annotations and imports change; no executable statement is touched and the materialized-view logic is untouched. Both files now differ from main only by the PgMatviewMixin import and its position in the base class list.
|
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 findingsAdds materialized view discovery to the PostgreSQL and Greenplum connectors by mapping relkind 'm' and updating view-path queries, addressing the matview lookup failure issue. No issues found. ✅ 1 resolved✅ Edge Case: Matview lookup failure drops all views for the schema
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 |
|
🚦 Removed from the merge queue —
|
46fda2d



Describe your changes:
Fixes #31515
The PostgreSQL and Greenplum ingestion connectors do not discover materialized views because two things were missing:
RELKIND_MAPincommon_pg_mappings.pyhad no'm'entry, so therelkindvalue returned for materialized views had no mapping toTableType.MaterializedView.POSTGRES_GET_TABLE_NAMESandGREENPLUM_GET_TABLE_NAMESonly filtered onrelkind in ('r', 'p', 'f'), which excludes'm'(materialized view) entirely.I added
"m": TableType.MaterializedViewtoRELKIND_MAPand extended both queries to include'm'in therelkindfilter.TableType.MaterializedViewalready exists in the generated schema (used by Snowflake, ClickHouse, BigQuery, and others), so no schema changes are needed.Type of change:
High-level design:
N/A — small change.
The
pg_class.relkindcolumn uses'm'for materialized views per the PostgreSQL catalog docs. The existing comment in both query files already listedm = materialized viewbut the filter never matched it.Tests:
Use cases covered
Unit tests
ingestion/tests/unit/topology/database/test_postgres.pytest_relkind_map_includes_materialized_view— assertsRELKIND_MAP['m'] == TableType.MaterializedViewtest_postgres_get_table_names_includes_materialized_view_relkind— assertsPOSTGRES_GET_TABLE_NAMEScontains'm'Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
Verified by code inspection:
TableType.MaterializedViewexists inmetadata.generated.schema.entity.data.table.RELKIND_MAP['m']resolves toTableType.MaterializedViewafter change.POSTGRES_GET_TABLE_NAMESnow includes'm'in therelkind in (...)filter.GREENPLUM_GET_TABLE_NAMESnow includes'm'in therelkind in (...)filter.UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #31515above.Greptile Summary
This PR routes PostgreSQL and Greenplum materialized views through the existing view-discovery path and classifies them as
MaterializedView.includeViewssemantics.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Reviews (8): Last reviewed commit: "Fix ci errors" | Re-trigger Greptile
Context used: