Skip to content

Fixes 31478: re-scope the Data Insights data asset charts in 1.13.6 SQL [1.13] - #33052

Open
ulixius9 wants to merge 1 commit into
1.13from
backport-31478-113
Open

Fixes 31478: re-scope the Data Insights data asset charts in 1.13.6 SQL [1.13]#33052
ulixius9 wants to merge 1 commit into
1.13from
backport-31478-113

Conversation

@ulixius9

@ulixius9 ulixius9 commented Sep 9, 2026

Copy link
Copy Markdown
Member

Describe your changes:

Fixes #31478

Backport of #33049 to 1.13 (cherry-pick -x of 3feeace927). This is the branch the bug is actually observable onhttps://release-1-13.getcollate.io/data-insights/data-assets still serves the pre-fix chart definitions.

#31759 fixed the scope of the data-asset charts but shipped the rewrite as a Java data migration hung off 1.13.5, and no deployment ran it: 1.13.5-release had already shipped and been recorded, and MigrationWorkflow never reaches runDataMigration() for a recorded version once it is no longer the release train's highest. The symptom is visible in the breakdown — it still shows a Data Products bucket that the migrated excludeGroups excludes — and the summary card sits 28 above the sum of the buckets.

This replaces that with guarded SQL in 1.13.6. runSchemaChanges/runPostDDLChanges are not behind shouldRunDataMigration, and parseSQLFiles keeps any statement whose checksum is absent from SERVER_MIGRATION_SQL_LOGS, so the repair runs on a deployment that has already recorded 1.13.6 — which is the case for every 1.13 nightly, since 1.13.6 landed on this branch on 2026-09-07 (c9819050c3) with real SQL. See #33049 for the full design.

Type of change:

  • Bug fix

High-level design:

See #33049. Two divergences from the main PR, both deliberate:

  1. 1.13.6/{mysql,postgres}/schemaChanges.sql are left exactly as they are on this branch. They already carry unrelated statements from c9819050c3; only postDataMigrationSQLScript.sql (empty on this branch) is filled in. Verified: those two files do not appear in git diff origin/1.13 --name-only.
  2. DataAssetChartScopeSqlMigrationTest builds the Flyway dialect parser locally instead of calling MigrationFile.parseSQLFile(File, ConnectionType), which does not exist on this branch — here the parsing is inlined in the instance method parseSQLFiles() behind a checksum filter that needs a live MigrationDAO. The helper mirrors that method's parser setup minus the filter, so the check (each file parses into exactly 2 statements with the literals intact) is preserved rather than dropped.

DataInsightSystemChartResource is untouched on this branch — it carries no startup hook.

Tests:

Use cases covered

Same as #33049:

  • "Total Data Assets" equals the sum of its per-entity-type breakdown when test case results exist in the window
  • The breakdown reports only genuine data assets (no tag, glossaryTerm or dataProduct bucket)
  • A deployment that has already recorded 1.13.6 is still repaired, exactly once
  • An already-corrected definition, and any user-created chart, are left untouched

Unit tests

  • I added unit tests for the new/changed logic.
  • Files: .../migration/utils/v1136/DataAssetChartScopeSqlMigrationTest.java (new, 13 tests), .../migration/MigrationWorkflowReprocessingTest.java (+4 cases), .../test/resources/migration/v1136/*.json (new). Removed .../migration/utils/v1135/DataInsightChartMigrationTest.java along with the Java migration it covered.
  • Result, run on this branch: mvn test -pl openmetadata-service -Dtest=DataAssetChartScopeSqlMigrationTest,MigrationWorkflowReprocessingTest,DataAssetChartScopeTestTests run: 37, Failures: 0, Errors: 0. spotless:check clean.
  • DataAssetChartScopeTest (Fixes 31478: stop Data Insights summary cards counting test case results #31759's real-OpenSearch regression test) passes here too — note it silently reports Tests run: 0 without a Docker daemon, via assumeTrue.
  • No measured coverage figure is claimed (no mvn jacoco:report); the changed Java surface is one extracted accessor in DataInsightSystemChartRepository, and the behaviour of this PR lives in SQL, which the tests above cover directly.

Backend integration tests

  • Not applicable — no API endpoint added or changed.

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright (UI) tests

  • Not applicable (no UI changes).

Manual testing performed

The SQL was executed end to end against the engine versions this repo ships (mysql:8.0.46, postgres:15.19) on the 1.5.0 di_chart_entity DDL, seeded with 8 rows: 5 pre-fix definitions, 1 already-migrated, 1 user-created chart grouped on entityType.keyword, and 1 with two metrics. Both engines reported identical row counts (UPDATE 5 filters, UPDATE 2 breakdown groups), then UPDATE 0/UPDATE 0 on replay, converging on identical JSON with filter as a JSON string and excludeGroups as an array. The two-metric definition was skipped by the guard, not half-rewritten; the already-migrated row and the user's chart were byte-identical.

The rollout was also driven through the real MigrationWorkflow.loadMigrations() against a real MySQL and the real bootstrap/sql/migrations/native tree with every 1.x version through 1.13.6 recorded in SERVER_CHANGE_LOG: 1.13.6 is still offered, and drops out once its two statement checksums are logged. Full detail in #33049.

Not covered: runMigrationWorkflows() itself (selection and execution were each driven against real databases, but not in one process — computeMigrationContext needs the initialized entity registry). CI's containerized migration run covers that.

UI screen recording / screenshots:

Not applicable — no UI changes.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: not applicable — no schema change; this PR is the data migration.
  • For UI changes: not applicable.
  • I have added tests (unit / integration as applicable) and listed them above.
  • I have added a test that covers the exact scenario we are fixing, with the issue number commented in the test for future reference.

🤖 Generated with Claude Code

PR #31759 rewrote the stored chart definitions from a Java data migration hung
off 1.13.5. 1.13.5-release had already shipped and been recorded by then, and
MigrationWorkflow never reaches runDataMigration() for a recorded version once
it is no longer the release train's highest: processNativeMigrations drops it
outright, and even while it is the highest, filterAndGetMigrationsToRun skips a
reprocessing version whose SQL has all run. So no deployment ran it, and
release-1-13 still serves the pre-fix definitions — its breakdown shows a Data
Products bucket the migrated excludeGroups excludes, and the summary card sits
28 above the sum of the buckets.

Express the repair as SQL in 1.13.6 instead. runSchemaChanges and
runPostDDLChanges are not behind shouldRunDataMigration, and parseSQLFiles keeps
any statement whose checksum is absent from SERVER_MIGRATION_SQL_LOGS, so new
SQL in the train's highest version runs even where that version is already
recorded. That covers all three states: release-1-13 (1.13.6 recorded,
reprocessed), a customer whose max is 1.13.5 (1.13.6 fresh), and 2.x on main
(1.13.6 fresh). Both statements are guarded on the stored value, so a deployment
that already holds the corrected definitions is untouched.

The 1.13.5 Java migration and DataInsightChartMigration are removed;
DataInsightSystemChartRepository keeps DATA_ASSET_FILTER and
NON_DATA_ASSET_ENTITY_TYPES as the scope the aggregators and the OpenSearch
regression test read, and the SQL is pinned to them by test. Its SearchClient
moved out of a static initializer, which made the class NPE before the
application is up — that is also why the old migration's unit test only passed
when co-run with the OpenSearch test that installs a SearchRepository.

Verified end to end against the engine versions OpenMetadata ships (mysql:8.0.46
and postgres:15.19), on the 1.5.0 table DDL seeded with pre-fix definitions as
1.5.0/1.6.0 leave them. Both engines report identical row counts — 5 filter
rewrites and 2 breakdown rewrites, then 0 and 0 on a second run — and converge
on identical JSON, with filter stored as a JSON string and excludeGroups as an
array:
- the breakdown gains the filter and drops dataProduct; the cards' stale
  must_not filter is replaced; a chart grouped on tier.keyword gets the filter
  but is not given entity-type excludeGroups
- an already-migrated definition, and a user's own chart grouped on
  entityType.keyword, are both left byte-identical
- a definition with two metrics is skipped by the JSON_LENGTH/jsonb_array_length
  guard rather than half-rewritten

The rollout claim was also driven through the real MigrationWorkflow against a
real MySQL and the real bootstrap/sql/migrations tree: with 1.13.6 recorded in
SERVER_CHANGE_LOG it is still offered, and once its two statement checksums are
in SERVER_MIGRATION_SQL_LOGS it drops out — so the repair reaches an
already-upgraded deployment exactly once.

Tests:
- DataAssetChartScopeSqlMigrationTest: the SQL embeds DATA_ASSET_FILTER and the
  excludeGroups verbatim, both dialects scope the same 14 charts, every UPDATE is
  guarded, each file parses into exactly 2 statements through the same Flyway
  dialect parser the workflow uses, and the post-migration JSON captured from the
  real database deserializes into LineChart/SummaryCard with the scope applied.
- MigrationWorkflowReprocessingTest: the 1.13.4/1.13.5/1.13.6 rollout the Java
  migration was lost to, and that new SQL on the highest recorded version is what
  brings a version back.
- DataAssetChartScopeTest (real OpenSearch node): summary card equals the
  breakdown sum.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
(cherry picked from commit 3feeace)
@ulixius9
ulixius9 requested a review from a team as a code owner September 9, 2026 12:03
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

  • No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

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 skip-pr-checks label.

@github-actions github-actions Bot added Ingestion safe to test Add this label to run secure Github workflows on PRs labels Sep 9, 2026
@gitar-bot

gitar-bot Bot commented Sep 9, 2026

Copy link
Copy Markdown
Code Review ✅ Approved

Backports the SQL-based fix for Data Insights data asset charts to 1.13.6, replacing a Java migration from 1.13.5 that never executed. The new guarded SQL statements in postDataMigrationSQLScript.sql repair the chart scope on deployments that have already recorded 1.13.6, ensuring the breakdown excludes non-asset entity types and the summary total matches the bucket sum. Comprehensive unit tests confirm the fix handles already-corrected definitions and user-created charts without modification. No issues found.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ingestion safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant