jerryshao opened a new pull request, #13316: URL: https://github.com/apache/gravitino/pull/13316
> **Depends on #13307.** This branch is stacked on #13307, so the diff also shows its commits until #13307 is merged. Please review only the last 3 commits ([compare](https://github.com/jerryshao/gravitino/compare/fix-13304-13305-column-tags...fix-13309-13310-column-ids)). This PR stays a draft until #13307 is merged; I'll then rebase it onto `main` and mark it ready for review. ### What changes were proposed in this pull request? Tags, owners and privileges are attached to a column by its id. This PR fixes three paths that gave existing columns new ids or left column rows behind, so column-level metadata was lost. It also passes the right schema to authorization plugins on a cross-schema table rename. **Cross-schema table move with a column change (#13309)** - `TableColumnMetaService.updateColumnPOsFromTableDiff` now moves all of the table's column rows to the new `schema_id` whenever the table moves. Before, it did this only when no column changed, so a move plus a column change left the unchanged columns under the old schema, and a later cascade drop of that schema deleted them. **Re-import (#13309)** - `TableMetaService.insertTable(overwrite = true)` reuses the stored column ids by name, but only when the upsert kept the same table id and that table already existed. This is the case when a table is re-imported, e.g. loaded after it was renamed outside Gravitino. - A stored id is never given to a second column. - The tag and owner relations of stored columns that are gone are removed in the same transaction. - When the upsert resolves to another table's row, which MySQL/H2 can do through the `(schema_id, table_name)` key, no ids are reused, so that table's columns are not inherited. - `TableOperationDispatcher.importTable` returns the stored entity, which carries the ids the store actually kept. **Lance `VERSION_CHECK` schema refresh (#13309)** - `LanceTableOperations.replaceColumnsFromDataset` reuses the id, comment and audit info of each column that is still in the dataset. Type, nullability and position come from the dataset. **Cross-schema table rename and authorization (#13310)** - `TableHookDispatcher.alterTable` builds the new identifier from `RenameTable.getNewSchemaName()`. If the request has several renames, it takes the last one that sets a schema. - `AuthorizationUtils` gets a new `authorizationPluginRenamePrivileges` overload that takes the new `NameIdentifier`. The existing `String` overload delegates to it and behaves as before. ### Why are the changes needed? - Moving a table to another schema and changing a column in one `alterTable`, then dropping the old schema, deleted the unchanged columns and their tags, owners and privileges. - Re-importing a table, e.g. after an out-of-band rename, gave every column a new id, so all column tags were lost. - On a Lance catalog with `lance.schema-refresh-mode=VERSION_CHECK`, every dataset version change gave all columns new ids and cleared their comments. - On a cross-schema rename, authorization plugins such as Ranger were told the table moved to `<old schema>.<new name>`, which doesn't exist. Fix: #13309, #13310 Part of #13303 ### Does this PR introduce _any_ user-facing change? No API or configuration change. Behaviour changes: - Column tags, owners and privileges survive a table move with a column change, a re-import and a Lance schema refresh. - Lance columns keep their comments across a schema refresh. - Authorization plugins get the correct schema on a cross-schema table rename. Not changed here (tracked in #13303): when the MySQL/H2 upsert resolves to a stale registration with the same name, that registration's column relations are still orphaned. ### How was this patch tested? - **Unit tests** (H2 locally; each one fails without its fix): - `TestTableColumnMetaService`: - `testMoveTableWithColumnChangeMovesAllColumns` - `testOverwriteSameTableKeepsColumnIds` - `testOverwriteNeverGivesOneStoredIdToTwoColumns` - `testOverwriteWithoutColumnsRemovesAllColumnRelations` - `testOverwriteOfAnotherTableDoesNotInheritColumnIds` (MySQL/H2 only) - `TestLanceTableOperations.testVersionCheckRefreshKeepsExistingColumnIdsAndComments` - `TestTableHookDispatcher`: `testRenameAcrossSchemasPassesNewSchemaToAuthorization`, `testRenameTwiceKeepsSchemaFromEarlierRename` - `TestAuthorizationUtils.testRenameTableAcrossSchemasNotifiesAuthorizationPluginWithNewSchema` - **Integration tests**: - `CatalogGenericCatalogLanceIT.testVersionCheckRefreshKeepsColumnTagsAndComments`: passes locally and fails without the fix. - `CatalogHive2IT.testOutOfBandRenameKeepsColumnTags` (also runs in `CatalogHive3IT`): renames the table directly in HMS, then checks that the column keeps its tag. Needs Docker. - `TagIT.testMovedTableKeepsColumnTagsAfterOldSchemaIsDropped`: needs Docker. - `./gradlew :core:test -PskipITs` and the `catalog-lakehouse-generic` unit tests pass locally. The MySQL/PostgreSQL backends and the Docker-based ITs still need to run in CI. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
