Fixes #32978: preserve table column lineage during version consolidation - #32309
Conversation
…er pass, set refresh=false on updateColumnsInUpstreamLineage - EntityRepository: toLowerCase(Locale.ROOT) to avoid locale-sensitive mis-keying (e.g. Turkish locale) — matches columnMatch's equalsIgnoreCase - TableRepository: clear pendingRenameColumnFqns before each putAll to prevent conflicting A→B + B→A mappings when consolidateChanges runs updateInternal multiple times - ElasticSearchEntityManager / OpenSearchEntityManager: change refresh=true → refresh=false in updateColumnsInUpstreamLineage, consistent with deleteColumnsInUpstreamLineage — same rationale applies: rename lineage cleanup does not need immediate read-after-write consistency
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…olerance, rename-path IT - TableUpdater: override resetForRetryAttempt() so a deadlock replay re-enqueues the deferred column-lineage search flush. The retry prologue clears deferredReactOperations; without resetting the run-once guard the replayed attempt commits DB lineage but silently drops the search-index cleanup (the divergence #21536 originally fixed). Mirrors the override the other seven updaters already carry. - ES/OS EntityManagers: ignoreUnavailable(true) on both column-lineage updateByQuery calls — with 8 concrete index names one missing index aborted cleanup for all of them, where the old "all" alias simply skipped missing members. Rewrote the wrapped refresh comments and moved the OpenSearch delete-path comment before its statement. - SearchClient: document LineageRepository.getChildrenNames as the source of truth for COLUMN_LINEAGE_SEARCH_INDICES with a keep-in-sync note (13 other index mappings declare upstreamLineage but can never receive column lineage). - TableResourceIT: cover the rename path via a case-only column rename (columnMatch is equalsIgnoreCase, so the column matches while its FQN changes) — updateColumnsInUpstreamLineage previously had no coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
github-actions
Bot
added
backend
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
This PR addresses severe latency in PATCH /api/v1/tables/{id} by ensuring column-lineage search-index cleanup work is deduplicated, scoped to the relevant indices, and made non-blocking, while also reducing column-matching complexity during updates.
Changes:
- Scope column-lineage
updateByQueryoperations to a dedicated set of 8 indices (instead of the global"all"alias) and tolerate missing indices. - Make column-lineage
updateByQuerynon-blocking by disablingrefresh, and deduplicate deferred lineage cleanup so it flushes exactly once post-commit (including retry-safety). - Optimize column matching in
ColumnEntityUpdater.updateColumnsby replacing per-column stream scans with a hashmap lookup; add integration coverage for delete + rename propagation to search.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/search/SearchClient.java | Adds COLUMN_LINEAGE_SEARCH_INDICES to target only indices that can hold column-level lineage. |
| openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchEntityManager.java | Uses ignoreUnavailable(true) and refresh=false for column-lineage update-by-query operations. |
| openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchEntityManager.java | Uses ignoreUnavailable(true) and refresh=false for column-lineage update-by-query operations. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TableRepository.java | Accumulates pending rename/delete FQNs and defers a single post-commit flush; resets guard state for retry attempts. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java | Replaces O(n²) column matching with a hashmap lookup keyed by a ColumnKey record. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TableResourceIT.java | Adds IT coverage for delete + rename lineage propagation into search; introduces a helper to query upstreamLineage from the index. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Review (greptile, gitar) caught a regression in the deferred-flush rewrite: clearing the pending collections on every pass meant the flush read the FINAL consolidation pass, but revert() rebases that pass onto the pre-session version. The search index sits at `original`, so the final pass's FQNs match no indexed document — and a rename away and back within the session yields a net-zero final diff, leaving the flush empty and the index stranded on the intermediate FQN while the DB moved on. Accumulate from the first pass instead, which is the only one that diffs against the state the index reflects (and the sole pass when consolidation does not apply). This also keeps the revert pass out of the flush, so a column added earlier in the session is no longer surfaced as a delete — the spurious-delete hazard that exists on main, where every pass enqueued its own search op. resetForRetryAttempt() zeroes the pass counter so a deadlock replay starts from the baseline pass. Also from review: - putIfAbsent in the origColumns lookup map, preserving the first-match semantics of the stream findAny() it replaced on duplicate keys - close the search response stream in the IT helper, which the new polling tests exercise repeatedly Adds test_revertedColumnRenameWithinSessionPropagatesInSearch covering the net-zero consolidation path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Most column deletes and renames have nothing downstream referencing them, so updatedDocuments == 0 is the ordinary outcome during ingestion, not an anomaly. Warning on it produced noisy and potentially alert-triggering logs on a routine path. WARN is now reserved for version conflicts and ERROR for failures. The case still carries a diagnostic value it is worth keeping the message for -- it is also what a missing index or a misresolved index selector looks like -- but that failure mode is pinned at build time by ColumnLineageSearchIndicesTest, which ties the selector to the resolver registry, so it does not need a runtime warning to catch it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core entity-update/lineage reconciliation behavior across persistence and search backends, warranting final human verification despite strong test coverage.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
openmetadata-service/src/test/java/org/openmetadata/service/search/ColumnLineageSearchIndicesTest.java:69
- This assertion hardcodes the index name separator as "" ("clusterx") even though the separator is defined centrally as IndexMapping.INDEX_NAME_SEPARATOR. Using the constant makes the test resilient if the separator ever changes (or if a different separator is introduced for other index classes), while keeping the same intent.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The performance fix is correctly scoped, preserves lineage correctness via baseline-pass gating, and is backed by meaningful integration tests covering delete/rename/revert and bulk/nested scenarios.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Column-lineage reconciliation update-by-query still forces a blocking refresh in both ES and OpenSearch managers, which reintroduces the primary latency driver this PR is intended to remove.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/ColumnLineageReconciliationIT.java:121
- Add an explicit index refresh helper so tests can control visibility deterministically when refresh_interval is disabled (used here to model stale snapshots / version conflicts).
private JsonNode readSource() throws IOException {
return request("GET", "/_doc/target", null).path("_source");
}
- Files reviewed: 17/17 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
ColumnLineageReconciler currently disables retries too broadly for rename maps (can skip safe retries and leave avoidable version conflicts), so the retry-safety logic should be corrected before approval.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
openmetadata-service/src/main/java/org/openmetadata/service/search/ColumnLineageReconciler.java:29
retrySafedisables conflict retries whenever any rename target is also a rename source (renames.values().stream().noneMatch(renames::containsKey)), which blocks retries for safe rename chains (e.g. a->b, b->c) even though the comment only calls out direct swaps (a->A, A->a). This can leave avoidable version conflicts unreconciled.
openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchEntityManager.java:943- The comment above
refresh(!renames.isEmpty())says conflict retries cannot recover without a refresh, but conflict retries already callindices().refresh(...)viaColumnLineageReconciler.reconcile(...). As written, the comment is misleading about why refresh is needed here.
openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchEntityManager.java:1007 - The comment above the conditional refresh claims conflict retries cannot recover without it, but retries already refresh explicitly in
ColumnLineageReconciler.reconcile(...). This comment should explain the actual reason (making renamed FQNs searchable for subsequent update-by-query calls) to avoid misleading future changes.
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
Restore predicate-based column matching, the global search alias, and synchronous refreshes. Remove the lookup and index-selection abstractions and the tests specific to those optional optimizations. Retain persisted-baseline lineage reconciliation, nested change collection, combined search updates, and bounded conflict recovery.
|
Narrowed this PR in 7be9b80 following the reporter's clarification about slow OpenSearch storage. The PR retains the independently justified lineage fixes: apply mutations only against the persisted baseline, collect nested column changes, reconcile renames/deletions together after commit, and handle version conflicts with bounded retries and accurate failure reporting. The optional optimizations are removed: column matching uses the original predicate scan; search cleanup uses the existing global alias; both engines retain synchronous refresh. The lookup-key and index-selection abstractions and their dedicated tests are removed as well. Validation on the narrowed diff: 12 integration cases passed on MySQL 8.3/Elasticsearch 9.3, 12 passed on PostgreSQL 15/OpenSearch 3.4, and all 6 retry unit tests passed. The clean backend build, Spotless, and pre-commit checks passed. A control that disables only the persisted-baseline guard fails the session-added-column lineage-preservation test; the same test passes with the guard on both stacks. The title and description now target the reproduced correctness bug, #32978. #26674 remains related; this PR makes no claim to resolve the storage-related latency or to deliver a measured end-to-end performance improvement. The required Shipping metadata for #32978 is still pending because the current GitHub token has read-only project access. The corresponding values on the original tracking issue are Status: In Review / QA, Source: OSS, Priority: P1, Domain: Platform, Release: 2.1.0. |
There was a problem hiding this comment.
🟡 Changes recommended
The new reconciliation path still forces refresh and still targets the global “all” alias in the changed code paths, which undermines the stated performance/scoping goals and risks reintroducing the original latency.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Lite
|
The Collate failure in run 34251264490 comes from the PR branch missing an upstream validation change.
The next step is to sync this branch with main and rerun the Collate compatibility check. This failure requires no change to the lineage implementation or to the test expectation. |
Bring in OpenMetadata #29566, which Collate's existing integration test requires. Preserve the table column lineage fix without changing its scope.
|
Merged OpenMetadata main ( Validation on the merged tree:
The push triggers a fresh Collate compatibility check against the updated commit. No additional Collate PR or test-expectation change is needed. |
There was a problem hiding this comment.
🟡 Changes recommended
A null deleted-column FQN can currently trigger an NPE during lineage change aggregation, which would break PATCH/PUT flows on legacy data.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
Skip null FQNs before collecting deleted columns for lineage reconciliation so legacy rows do not fail PATCH and PUT with List.copyOf's null rejection. Cover both API paths with a legacy row and verify that valid deleted columns still lose their stored and indexed lineage.
Code Review ✅ Approved 4 resolved / 4 findingsFixes table column lineage corruption during version consolidation by limiting lineage mutations to comparisons against the persisted table state and deferring search reconciliation until after commit. Addresses net-zero consolidated renames/deletes leaving search stale, HashMap column lookup keeping last match instead of first, and reconcile flush behavior. All integration tests pass on MySQL/Elasticsearch and PostgreSQL/OpenSearch stacks. ✅ 4 resolved✅ Bug: Net-zero consolidated rename/delete leaves search lineage stale
✅ Edge Case: HashMap column lookup keeps last match vs. findAny's first
✅ Performance: reconcile flush switched back to refresh=true on request thread
✅ Performance: Lineage flush reverts to GLOBAL_SEARCH_ALIAS cluster-wide scan
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.
🔵 Needs a closer look
It changes core entity update/consolidation behavior and search update-by-query reconciliation paths, so it warrants final human maintainer review despite strong targeted test coverage.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
Describe your changes:
Fixes #32978
A table column added earlier in the same editing session can lose its lineage during a later description-only PATCH. Version consolidation compares the persisted table with the older session baseline, which can report that still-existing column as deleted. This PR limits lineage mutations to the comparison against the actual persisted table and combines the nested column changes into one reconciliation after commit.
Related to #26674; continuation of #26848 by @khoaihps. The original contributor commits and authorship are preserved.
The reporter's follow-up identifies slow OpenSearch storage as the primary cause of the reported latency. This PR addresses the independent lineage correctness problem and redundant update work. End-to-end latency improvements have not been measured, and the original performance issue is not claimed fixed.
Type of change:
High-level design:
EntityUpdater, reset that state on transaction retry, and gate both database lineage changes and deferred search reconciliation on it. Historical comparisons still run for version consolidation.No schema, API contract, or migration changes. Search reconciliation remains best effort after commit: persistent conflicts or search outages are logged; durable repair is outside this change.
Tests:
Use cases covered
Unit tests
ColumnLineageReconcilerTest: 6 passed, covering retry bounds, refreshed snapshots, no-match/success, shard and transport failures, and overlapping renames. The newColumnLineageReconcilerhelper has 21/21 lines covered (100%, JaCoCo 0.8.13). Whole-class coverage for the existing repositories and search managers was not measured by this focused run.Backend integration tests
TableResourceIT(7 cases) andColumnLineageReconciliationIT(5 cases) exercise real database and search backends:Both suites ran against a clean build of the narrowed diff. The service artifact checksum was verified before and after execution.
Ingestion integration tests
Not applicable — no ingestion changes.
Playwright (UI) tests
Not applicable — no UI changes.
Manual testing performed
mvn clean install -pl openmetadata-service,openmetadata-integration-tests -am -DskipTests -Dcheckstyle.skip: all 11 reactor modules built successfully on Java 21.mvn spotless:apply -pl openmetadata-service,openmetadata-integration-testsandpre-commit run: passed; no remaining formatting diff.TableRepository, restoring main's unconditional lineage-update behavior. The session-added-column test failed at the lineage-preservation assertion after the description-only PATCH (1 test started, 1 expected failure, no aborted tests). The same test passed on both stacks with the guard enabled.No end-to-end ingestion performance benchmark or full backend test-suite run was performed locally.
UI screen recording / screenshots:
Not applicable — no UI changes.
Checklist:
Fixes #32978.Related to #26674is intentional: this change does not establish a fix for the reported slow-storage incident.Fixes <issue-number>: <short explanation>and describes the lineage correctness fix.