[#12382] feat(lance): Support AddColumn via Gravitino API - #12383
[#12382] feat(lance): Support AddColumn via Gravitino API#12383bbiiaaoo wants to merge 2 commits into
Conversation
Code Coverage Report
Files
|
Code reviewFound 1 issue:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
|
@FANNG1 Thanks for catching this. Fixed by hydrating the Lance schema before handling AddColumn and reloading the TableEntity afterward as the optimistic-lock snapshot. Added regression tests for both declared tables and non-declared tables with empty stored columns. |
|
Hi @FANNG1 @yuqi1129 , would it be acceptable to simplify the AddColumn implementation by following the existing Lance alter-table flow? The proposed flow is:
This would remove the custom metadata CAS and physical rollback logic, and would provide the same best-effort consistency model as the existing DeleteColumn/RenameColumn paths. Would this simpler approach be acceptable for the first-phase AddColumn support? |
It's fine currently. In fact, we need to review the Lance directory as the single source of truth; no matter how much effort we put in, there is still a high possibility that the data in Gravitino and Lance differ. |
| try { | ||
| return persistAddedColumns(ident, loadedEntity, changes, version); | ||
| } catch (RuntimeException metadataFailure) { | ||
| rollbackAddedColumns(loadedTable, fieldsToAdd, metadataFailure); |
There was a problem hiding this comment.
Would you roll back the column created in the Lance dataset when writing column information fails?
yuqi1129
left a comment
yuqi1129
left a comment
There was a problem hiding this comment.
Thanks for the work here! The add/verify/rollback sequence itself looks correct (I checked that Dataset.checkoutLatest() is in-place and getSchema() is not cached in lance-core 6.0.0). I left three comments, two of which I think can permanently break AddColumn on otherwise-healthy tables.
Also verified as fine, in case it saves anyone else the trip: no double rollback on the persistAddedColumns path, ColumnPosition.defaultPos() is a singleton so the equals position check holds over REST, appendAddedColumns position assignment is safe because ManagedTableOperations.applyChanges re-indexes, and OptimisticLockException maps to 409 rather than 500.
|
|
||
| private long addColumns( | ||
| Table table, List<Field> fieldsToAdd, String location, Map<String, String> storageOptions) { | ||
| List<Field> expectedCurrentFields = convertColumnsToArrowSchema(table.columns()).getFields(); |
There was a problem hiding this comment.
The "expected" schema is rebuilt from Gravitino metadata through LanceDataTypeConverter and then compared to the live Lance schema with an exact positional field comparison (see fieldMatches below). But toGravitino -> fromGravitino is not round-trip lossless, so several classes of perfectly consistent tables will fail the pre-check at line 847 and never be able to add a column again:
- Timestamp with time zone:
toGravitinomaps any tz toTimestampType.withTimeZone(p), andfromGravitinoalways re-emitsTimestamp(unit, "UTC")(the converter even carries// todo: need timeZoneId for timestamp with time zone). A dataset withtimestamp[us, tz=Asia/Shanghai]never matches. - List:
toGravitinodrops the list child field name, andtoArrowFieldhard-codes it back as"element", while datasets written by pyarrow / arrow-rs name it"item". - Decimal:
fromGravitinoalways emits bit width 128, so adecimal256column never matches.
The resulting OptimisticLockException surfaces as HTTP 409, which tells the client to retry, but the condition is permanent — under the default DECLARED_AND_EMPTY refresh mode a non-empty table is never re-hydrated, so there's no supported way to clear it.
The same strict comparison is reused for the post-add check at line 851, which has the mirror problem: if Lance normalizes the written field in any way, an add that actually succeeded gets rolled back and reported as a failure.
Would it be possible to compare against the schema actually read from the dataset (e.g. snapshot it before the add and diff that against the post-add schema), rather than against a re-derived one?
| try { | ||
| return persistAddedColumns(ident, loadedEntity, changes, version); | ||
| } catch (RuntimeException metadataFailure) { | ||
| rollbackAddedColumns(loadedTable, fieldsToAdd, metadataFailure); |
There was a problem hiding this comment.
This rollback can leave Gravitino metadata listing columns that no longer exist physically.
persistAddedColumns fails the CAS whenever !current.equals(expectedEntity). With lance.schema-refresh-mode=VERSION_CHECK, a concurrent loadTable landing in the window between dataset.addColumns() committing and the CAS running will hydrate the new schema into the entity store and bump lance.version via repairTableMetadata. That both (a) makes this CAS fail and (b) means the store already contains the added columns. We then call rollbackAddedColumns, which physically drops them from the dataset, and rethrow — net result is metadata referencing columns Lance doesn't have, and under the default DECLARED_AND_EMPTY mode a non-empty table is never re-checked, so it stays that way.
The dispatcher only takes a READ tree-lock for non-rename changes (TableOperationDispatcher.alterTable), so this interleaving isn't excluded.
One option: make the CAS compare only the fields that actually matter (columns + lance.version) and skip the physical rollback when the stored schema already reflects the add.
| return true; | ||
| } | ||
|
|
||
| private boolean fieldMatches(Field expected, Field actual) { |
There was a problem hiding this comment.
Minor / lower severity, but it lands on the same path: column types Lance/Arrow can't map back to a Gravitino type — notably fixed_size_list vector columns, arguably the flagship Lance type — are stored as Types.ExternalType whose catalogString is the serialized Arrow Field, including its original name.
RenameColumn is supported for Lance tables and rewrites only the ColumnEntity name, leaving the embedded JSON name stale. convertColumnsToArrowSchema at line 841 then hits the EXTERNAL branch of toArrowField, which does Preconditions.checkArgument(name.equals(field.getName()), "expected field name %s but got %s"). So after renaming a vector column, every subsequent AddColumn on that table dies with a confusing IllegalArgumentException before any Lance work happens.
Either rewrite the embedded name on rename, or override the name when reconstructing the Arrow field from ExternalType.
|
+1 to reusing the existing Lance One architectural concern remains: I think that broader reconciliation change can be handled in a separate PR to keep this one focused. What do you think? |
- reuse the existing Lance alter-table flow for AddColumn - batch nullable top-level columns into one Lance schema commit - hydrate declared or empty table metadata before adding columns - persist the Lance version and clear the declared marker - add validation, unit tests, integration tests, and documentation
|
Thanks @FANNG1 and @yuqi1129 for the suggestions. I have simplified and force-pushed the implementation based on the latest The updated implementation now:
I removed the strict Gravitino-to-Arrow schema reconstruction and comparison, custom metadata CAS, manual I agree that broader reconciliation from the Lance dataset back to Gravitino metadata should be handled in a separate PR, with Lance treated as the source of truth. Unit tests, the relevant integration tests, and the module check pass locally. Could you please take another look? |
|
|
||
| Set<String> columnNames = new HashSet<>(); | ||
| List<Field> fieldsToAdd = new ArrayList<>(changes.length); | ||
| for (TableChange change : changes) { |
There was a problem hiding this comment.
Is it possible to merge the loop above and the loop here?
There was a problem hiding this comment.
Thanks for the suggestion. I merged AddColumn detection, validation, and Arrow field construction into a single loop.
| metadataChanges[changes.length] = | ||
| TableChange.setProperty(LanceConstants.LANCE_TABLE_VERSION, String.valueOf(version)); | ||
| if (!fieldsToAdd.isEmpty()) { | ||
| metadataChanges[changes.length + 1] = |
There was a problem hiding this comment.
You can use ArrayUtils.add() directly with explicitly copying it.
There was a problem hiding this comment.
Another problem is why the index is changes.length + 1, not changes.length here?
There was a problem hiding this comment.
Done. I replaced the manual array copying and indexing with sequential ArrayUtils.add calls.
| // Adding all fields in one call creates one Lance schema version and backfills existing | ||
| // rows with null for the new nullable columns. | ||
| dataset.addColumns(fieldsToAdd); | ||
| dataset.checkoutLatest(); |
There was a problem hiding this comment.
How do you handle the case where there exist addColumn and dropColumn or AddIndex at the same time? Have you already excluded such scenarios?
There was a problem hiding this comment.
Mixed AddColumn and other table changes are rejected because the Lance physical schema update must be completed separately from the existing alter-table operations.
I added explicit tests for AddColumn combined with DeleteColumn in both orders, as well as AddColumn combined with AddIndex.
| } | ||
|
|
||
| @Test | ||
| public void testAlterTableAddsNullableColumnsInSingleLanceCommit() throws Exception { |
There was a problem hiding this comment.
Have you covered the case where we changed Lance to succeed but failed to write metadata to Gravitino, and then It's still okay when we load the table?
There was a problem hiding this comment.
I added a regression test for this failure window.
If the Lance add-columns commit succeeds but the Gravitino metadata update fails, a subsequent load in VERSION_CHECK mode detects the changed Lance version and refreshes both the schema and version from the underlying dataset.
Under the default DECLARED_AND_EMPTY mode, an existing non-empty stored schema is not automatically reconciled. Broader reconciliation in that mode can be addressed separately as part of the Lance-as-source-of-truth follow-up work.
- validate AddColumn changes and build Arrow fields in one pass - use ArrayUtils.add for internal metadata changes - add coverage for mixed AddColumn operations - verify version-check recovery after a metadata update failure
3 participants
What changes were proposed in this pull request?
This PR adds first-phase
AddColumnsupport for Lance tables through the Gravitino table API.The implementation:
AddColumnchanges into oneDataset.addColumnscall.NULLbackfill for existing rows.super.alterTableto persist the requested columns,lance.version, and removelance.declared.The implementation follows the existing Lance alter-table consistency model. It does not introduce custom metadata CAS, physical rollback, or strict schema reconstruction.
Why are the changes needed?
Lance tables currently cannot add columns through the Gravitino table API.
This change provides initial AddColumn support while keeping Lance as the source of truth and keeping broader Lance-to-Gravitino schema reconciliation outside the scope of this PR.
Fix: #12382
Does this PR introduce any user-facing change?
Yes.
Users can add nullable, top-level columns to Lance tables through the Gravitino table API. Existing rows are backfilled with
NULL.The Lance REST
/add_columnsendpoint is not included in this change.How was this patch tested?
NULLand multiple columns produce one Lance version.loadTable.Commands:
./gradlew spotlessApply./gradlew :catalogs:catalog-lakehouse-generic:test --tests org.apache.gravitino.catalog.lakehouse.lance.TestLanceTableOperations -PskipITs./gradlew :catalogs:catalog-lakehouse-generic:check -PskipITs