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 21 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! |
| const tableColumns = useMemo(() => { | ||
| const offset = (currentPage - 1) * pageSize; | ||
|
|
||
| return filteredHistoricalColumns.slice(offset, offset + pageSize); | ||
| }, [filteredHistoricalColumns, currentPage, pageSize]); | ||
|
|
||
| return getColumnsDataWithVersionChanges<Column>(changeDescription, colList); | ||
| const columns = useMemo(() => { | ||
| return getColumnsDataWithVersionChanges<Column>( | ||
| changeDescription, | ||
| cloneDeep(tableColumns) | ||
| ); | ||
| }, [tableColumns, changeDescription]); |
There was a problem hiding this comment.
getColumnsDataWithVersionChanges() can inject added/deleted columns into the provided list (e.g., deleted columns are unshifted via addDeletedColumnsDiff). Because pagination (slice) is done before applying version diffs, deleted/added columns may never be visible (or may appear on the wrong page) and page sizing can become inconsistent. Consider applying version changes to the full historical column list first, then performing search filtering and finally slicing for pagination.
| if (searchText && !handelSearchCallback) { | ||
| const searchCols = searchInColumns<T>(columns, searchText); | ||
|
|
||
| return makeData<T>(searchCols); | ||
| } else { | ||
| return makeData<T>(columns); | ||
| } | ||
| }, [searchText, columns]); | ||
|
|
||
| return makeData<T>(columns); | ||
| }, [searchText, columns, handelSearchCallback]); |
There was a problem hiding this comment.
The prop name handelSearchCallback appears to be a typo ("handel" vs "handle"). Since it’s now part of the core search/pagination flow, consider renaming it to handleSearchCallback (optionally keeping handelSearchCallback as a deprecated alias) to avoid spreading the misspelling.
|
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 findingsUpdates table version history to display historical datatypes and ensures the columnsLoading state is correctly reset, resolving the loading toggle issue. ✅ 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 |
|
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! |
|
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! |
|
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! |
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
Summary by Gitar
onLoadMore,hasMore, andisLoadingMoreprops toTableVersioncomponent.EntityVersionTimeLinecomponent to support version pagination.This will update automatically on new commits.