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 9 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! |
miantalha45
changed the title
|
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! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
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! |
|
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! |
| const filteredHistoricalColumns = useMemo(() => { | ||
| if (!searchText) { | ||
| return allHistoricalColumns; | ||
| } | ||
| const lower = searchText.toLowerCase(); | ||
|
|
||
| setTableColumns(pruneEmptyChildren(response.data) || []); | ||
| handlePagingChange(response.paging); | ||
| } catch { | ||
| // Set empty state if API fails | ||
| setTableColumns([]); | ||
| handlePagingChange({ | ||
| offset: 1, | ||
| limit: pageSize, | ||
| total: 0, | ||
| }); | ||
| } finally { | ||
| setColumnsLoading(false); | ||
| } | ||
| }, | ||
| [tableFqn, pageSize] | ||
| ); | ||
| return allHistoricalColumns.filter( | ||
| (col) => | ||
| col.name?.toLowerCase().includes(lower) || | ||
| col.description?.toLowerCase().includes(lower) | ||
| ); | ||
| }, [allHistoricalColumns, searchText]); |
There was a problem hiding this comment.
filteredHistoricalColumns only matches on top-level name and description, and it is not recursive. This changes search behavior vs VersionTable’s searchInColumns (which searches displayName, dataType, and children). Consider reusing the same searchInColumns logic (or an equivalent recursive filter) for the full historical column tree before slicing, so nested/typed matches aren’t dropped.
| total: filteredHistoricalColumns.length, | ||
| }); | ||
| setColumnsLoading(false); | ||
| }, [isVersionLoading, filteredHistoricalColumns, currentPage, pageSize]); |
There was a problem hiding this comment.
The useEffect dependency array omits handlePagingChange even though it’s used inside the effect. This is likely to trigger react-hooks/exhaustive-deps lint warnings and can capture a stale reference if the hook implementation changes. Add handlePagingChange to the dependency list (or refactor to avoid the effect).
| }, [isVersionLoading, filteredHistoricalColumns, currentPage, pageSize]); | |
| }, [ | |
| isVersionLoading, | |
| filteredHistoricalColumns, | |
| currentPage, | |
| pageSize, | |
| handlePagingChange, | |
| ]); |
| limit: pageSize, | ||
| total: filteredHistoricalColumns.length, | ||
| }); | ||
| setColumnsLoading(false); |
There was a problem hiding this comment.
columnsLoading is now driven entirely by synchronous local filtering/slicing, but it still starts as true and is set to false in an effect. This will render the schema table in a loading state briefly even when isVersionLoading is false. Consider removing columnsLoading state entirely (or initialize it to false and only use it when truly async work is happening).
|
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! |
Code Review ✅ Approved 1 resolved / 1 findingsHistorical table versions now correctly display the associated datatype, resolving a discrepancy where only the current type was shown. The columnsLoading state correctly toggles back to true to ensure consistent data fetching. ✅ 1 resolved✅ Quality: columnsLoading state is never set back to true
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
| const filteredHistoricalColumns = useMemo(() => { | ||
| if (!searchText) { | ||
| return allHistoricalColumns; | ||
| } | ||
|
|
||
| // Use search API if there's a search query, otherwise use regular pagination | ||
| const response = searchQuery | ||
| ? await searchTableColumnsByFQN(tableFqn, { | ||
| q: searchQuery, | ||
| limit: pageSize, | ||
| offset: offset, | ||
| fields: 'tags', | ||
| }) | ||
| : await getTableColumnsByFQN(tableFqn, { | ||
| limit: pageSize, | ||
| offset: offset, | ||
| fields: 'tags', | ||
| }); | ||
|
|
||
| setTableColumns(pruneEmptyChildren(response.data) || []); | ||
| handlePagingChange(response.paging); | ||
| } catch { | ||
| // Set empty state if API fails | ||
| setTableColumns([]); | ||
| handlePagingChange({ | ||
| offset: 1, | ||
| limit: pageSize, | ||
| total: 0, | ||
| }); | ||
| } finally { | ||
| setColumnsLoading(false); | ||
| } | ||
| }, | ||
| [tableFqn, pageSize] | ||
| ); | ||
| return searchInColumns(allHistoricalColumns, searchText); | ||
| }, [allHistoricalColumns, searchText]); |
There was a problem hiding this comment.
VersionTable already performs local column filtering based on its own searchText state, and this component now filters again via filteredHistoricalColumns. This duplicates work and can cause brief UI inconsistency (e.g., user types a search term and the child filters the current page slice before the parent updates pagination/slice). Consider making a single source of truth for filtering—either let TableVersion own search + pagination and disable the internal filtering in VersionTable for this usage, or pass the full (unfiltered) columns to VersionTable and let it handle filtering while TableVersion only handles pagination totals/offsets.
| useEffect(() => { | ||
| if (tableFqn && !isVersionLoading) { | ||
| // Reset to first page when search changes | ||
| fetchPaginatedColumns(currentPage, searchText || undefined); | ||
| if (isVersionLoading) { | ||
| return; | ||
| } | ||
| }, [ | ||
| isVersionLoading, | ||
| tableFqn, | ||
| searchText, | ||
| fetchPaginatedColumns, | ||
| pageSize, | ||
| currentPage, | ||
| ]); | ||
| const offset = (currentPage - 1) * pageSize; | ||
| setTableColumns(filteredHistoricalColumns.slice(offset, offset + pageSize)); | ||
| handlePagingChange({ | ||
| offset, | ||
| limit: pageSize, | ||
| total: filteredHistoricalColumns.length, | ||
| }); | ||
| }, [isVersionLoading, filteredHistoricalColumns, currentPage, pageSize, handlePagingChange]); |
There was a problem hiding this comment.
tableColumns and the paging totals are only populated in a useEffect, so the first paint after isVersionLoading becomes false can render an empty VersionTable (and showPagination may be wrong) for one render cycle. To avoid a visible empty-state flash, initialize the paginated columns/paging synchronously (e.g., compute the slice in a useMemo and pass it directly, or initialize state from currentVersionData.columns, or switch this effect to useLayoutEffect).
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