Fixes #27162: table version history shows current datatype instead of historical one#27478
Fixes #27162: table version history shows current datatype instead of historical one#27478miantalha45 wants to merge 5 commits intoopen-metadata:mainfrom
Conversation
|
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! |
| limit: pageSize, | ||
| total: filteredHistoricalColumns.length, | ||
| }); | ||
| setColumnsLoading(false); |
There was a problem hiding this comment.
💡 Quality: columnsLoading state is never set back to true
columnsLoading is initialized to true and set to false once the useEffect runs, but it's never set back to true on subsequent data changes (search/page changes). Since column slicing is now synchronous, columnsLoading is effectively unnecessary. Consider removing it or, if the loading skeleton is visually desirable on initial render, adding a comment explaining that it's only used for the initial mount.
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
There was a problem hiding this comment.
Pull request overview
Fixes table version-history schema rendering so historical column dataTypeDisplay values come from the version snapshot (currentVersionData.columns) instead of the live “current table columns” API, addressing #27162.
Changes:
- Removed live table-columns API usage from
TableVersionand replaced it with local filtering + pagination overcurrentVersionData.columns. - Added a unit test asserting
VersionTablereceives historicaldataTypeDisplayfromcurrentVersionData.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/src/components/Database/TableVersion/TableVersion.component.tsx | Switches columns source to version snapshot and adds local search/pagination logic. |
| openmetadata-ui/src/main/resources/ui/src/components/Database/TableVersion/TableVersion.test.tsx | Adds test ensuring historical column datatype display is passed through to VersionTable. |
Comments suppressed due to low confidence (1)
openmetadata-ui/src/main/resources/ui/src/components/Database/TableVersion/TableVersion.component.tsx:137
paginationProps.isNumberBasedis currently set toBoolean(searchText), but pagination is now always local/offset-based (no cursor-based API). WhensearchTextis empty (default case),NextPrevioustreats paging as cursor-based and disables both Next/Previous becausepaging.after/paging.beforeare undefined, effectively breaking column pagination. SetisNumberBasedtotrueunconditionally for this view (or populatepaging.after/beforeappropriately, but number-based is the natural fit for local slicing).
const paginationProps = useMemo(
() => ({
currentPage,
showPagination,
isLoading: columnsLoading,
isNumberBased: Boolean(searchText),
pageSize,
paging,
pagingHandler: handleColumnsPageChange,
onShowSizeChange: handlePageSizeChange,
|
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! |
| setTableColumns(filteredHistoricalColumns.slice(offset, offset + pageSize)); | ||
| handlePagingChange({ | ||
| offset, | ||
| limit: pageSize, | ||
| total: filteredHistoricalColumns.length, |
There was a problem hiding this comment.
The local paging state is updated with { offset, limit, total } only, leaving paging.after/paging.before unset. However NextPrevious disables Next/Previous in cursor mode when paging.after/before are missing. Since paginationProps.isNumberBased is still Boolean(searchText) (cursor mode when search is empty), this change can make column pagination non-functional by disabling both buttons. Align the paging mode with the data you provide: either always use number-based paging here, or compute/set after/before cursors for local pagination.
| setTableColumns(filteredHistoricalColumns.slice(offset, offset + pageSize)); | |
| handlePagingChange({ | |
| offset, | |
| limit: pageSize, | |
| total: filteredHistoricalColumns.length, | |
| const total = filteredHistoricalColumns.length; | |
| const nextOffset = offset + pageSize; | |
| const previousOffset = Math.max(offset - pageSize, 0); | |
| setTableColumns(filteredHistoricalColumns.slice(offset, offset + pageSize)); | |
| handlePagingChange({ | |
| offset, | |
| limit: pageSize, | |
| total, | |
| before: offset > 0 ? String(previousOffset) : undefined, | |
| after: nextOffset < total ? String(nextOffset) : undefined, |
|
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 with suggestions 0 resolved / 1 findingsUpdates table version history to display historical datatypes by correctly accessing historical record state. Ensure columnsLoading is toggled back to true on subsequent data changes to prevent stale UI states. 💡 Quality: columnsLoading state is never set back to true
🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
Problem
When viewing an older version of a table, the column type column always
showed the current datatype (e.g.
decimal(15,3)) even if that versionhad a different type (e.g.
decimal(9,1)).Root Cause
TableVersionwas fetching columns viagetTableColumnsByFQN, whichhits the current table columns API. This meant
tableColumnsalwaysheld the latest data regardless of which version was being viewed. The
changeDescriptionfor the selected version was then applied on top ofwrong base data, so the historical type was never shown correctly.
Fix
Removed the live API call and replaced it with local pagination over
currentVersionData.columns. The version API already returns the fullhistorical entity snapshot including columns, so no extra network call
is needed. Search filtering is also done locally.
Test
Added a unit test that verifies
VersionTablereceives columns withthe historical
dataTypeDisplayvalue fromcurrentVersionData, notfrom any API response.
Proof
Fixes #27162