diff --git a/java/lance-jni/src/transaction.rs b/java/lance-jni/src/transaction.rs index fba2358d337..79309a5d401 100644 --- a/java/lance-jni/src/transaction.rs +++ b/java/lance-jni/src/transaction.rs @@ -26,7 +26,7 @@ use lance::io::commit::namespace_manifest::LanceNamespaceExternalManifestStore; use lance::table::format::{Fragment, IndexMetadata}; use lance_core::datatypes::Field; use lance_core::datatypes::Schema as LanceSchema; -use lance_file::version::LanceFileVersion; +use lance_file::version::{LanceFileVersion, V2_FORMAT_2_0, V2_FORMAT_2_1, V2_FORMAT_2_2}; use lance_io::object_store::{LanceNamespaceStorageOptionsProvider, StorageOptionsProvider}; use lance_table::io::commit::CommitHandler; use lance_table::io::commit::external_manifest::ExternalManifestCommitHandler; @@ -594,19 +594,38 @@ pub(crate) fn convert_to_java_schema<'local>( .l()?) } +/// Parse a `CommitBuilder.storageFormat` string into a [`LanceFileVersion`]. +/// +/// The canonical spellings ("2.1", "stable", ...) are the ones every other Lance +/// binding accepts and the ones [`LanceFileVersion`]'s `Display` emits. +/// +/// The `v`-prefixed spellings are a Java-only accident: this function originally +/// hand-rolled its match by walking the `LanceFileVersion` variant identifiers +/// (`V2_1` -> `"v2_1"`) instead of delegating to `FromStr`, so it accepted those +/// identifiers and rejected the canonical "2.1". They were documented on +/// `CommitBuilder.storageFormat` and shipped from 3.0.0, so they are translated +/// here for compatibility. The set is deliberately frozen to what shipped — +/// newer versions are reachable only by their canonical name. fn parse_storage_format(name: &str) -> Result { - match name.to_lowercase().as_str() { - "legacy" => Ok(LanceFileVersion::Legacy), - "v2_0" | "v2.0" => Ok(LanceFileVersion::V2_0), - "stable" => Ok(LanceFileVersion::Stable), - "v2_1" | "v2.1" => Ok(LanceFileVersion::V2_1), - "next" => Ok(LanceFileVersion::Next), - "v2_2" | "v2.2" => Ok(LanceFileVersion::V2_2), - _ => Err(Error::input_error(format!( - "Unknown storage format: {}", - name - ))), + let requested = name.to_lowercase(); + let canonical = match requested.as_str() { + "v2_0" | "v2.0" => V2_FORMAT_2_0, + "v2_1" | "v2.1" => V2_FORMAT_2_1, + "v2_2" | "v2.2" => V2_FORMAT_2_2, + _ => requested.as_str(), + }; + + if canonical != requested { + log::warn!( + "Storage format \"{}\" is deprecated and will be removed in a future release; use \"{}\" instead", + name, + canonical + ); } + + canonical + .parse::() + .map_err(|_| Error::input_error(format!("Unknown storage format: {}", name))) } /// Translate the Java `commitTimeoutNanos` sentinel into an @@ -1836,4 +1855,80 @@ mod tests { HashMap::from([("new_schema_k".to_string(), "new_schema_v".to_string())]) ); } + + #[test] + fn test_parse_storage_format_canonical_forms() { + let cases = [ + ("2.0", LanceFileVersion::V2_0), + ("2.1", LanceFileVersion::V2_1), + ("2.2", LanceFileVersion::V2_2), + ("2.3", LanceFileVersion::V2_3), + ("0.1", LanceFileVersion::Legacy), + ("legacy", LanceFileVersion::Legacy), + ("stable", LanceFileVersion::Stable), + ("next", LanceFileVersion::Next), + ]; + for (input, expected) in cases { + assert_eq!( + parse_storage_format(input).unwrap(), + expected, + "parse_storage_format({:?}) failed", + input + ); + } + } + + /// The `v`-prefixed spellings shipped in the `CommitBuilder.storageFormat` + /// Javadoc and must keep working for existing Java callers. + #[test] + fn test_parse_storage_format_deprecated_aliases() { + let cases = [ + ("v2_0", LanceFileVersion::V2_0), + ("v2.0", LanceFileVersion::V2_0), + ("v2_1", LanceFileVersion::V2_1), + ("v2.1", LanceFileVersion::V2_1), + ("v2_2", LanceFileVersion::V2_2), + ("v2.2", LanceFileVersion::V2_2), + ]; + for (input, expected) in cases { + assert_eq!( + parse_storage_format(input).unwrap(), + expected, + "parse_storage_format({:?}) failed", + input + ); + } + } + + /// The alias set is frozen to what shipped, so versions added after the + /// aliases were deprecated are reachable only by their canonical name. + #[test] + fn test_parse_storage_format_does_not_extend_aliases_to_new_versions() { + assert!(parse_storage_format("v2_3").is_err()); + assert!(parse_storage_format("v2.3").is_err()); + assert_eq!(parse_storage_format("2.3").unwrap(), LanceFileVersion::V2_3); + } + + #[test] + fn test_parse_storage_format_case_insensitive() { + assert_eq!( + parse_storage_format("LEGACY").unwrap(), + LanceFileVersion::Legacy + ); + assert_eq!( + parse_storage_format("Stable").unwrap(), + LanceFileVersion::Stable + ); + assert_eq!( + parse_storage_format("V2_1").unwrap(), + LanceFileVersion::V2_1 + ); + } + + #[test] + fn test_parse_storage_format_rejects_invalid() { + assert!(parse_storage_format("v3.0").is_err()); + assert!(parse_storage_format("").is_err()); + assert!(parse_storage_format("foo").is_err()); + } } diff --git a/java/src/main/java/org/lance/CommitBuilder.java b/java/src/main/java/org/lance/CommitBuilder.java index 62861b7ef73..fdb95828942 100644 --- a/java/src/main/java/org/lance/CommitBuilder.java +++ b/java/src/main/java/org/lance/CommitBuilder.java @@ -200,8 +200,14 @@ public CommitBuilder useStableRowIds(boolean useStableRowIds) { * Set the storage format to use for the dataset. * *

This is only needed when creating a new empty table. If any data files are passed, the - * storage format will be inferred from the data files. Valid values: "legacy", "v2_0", "stable", - * "v2_1", "next", "v2_2". + * storage format will be inferred from the data files. Valid values are the numeric versions + * ("0.1", "2.0", "2.1", "2.2", "2.3") and the release selectors ("legacy", "stable", "next"), + * matching {@link WriteParams.Builder#withDataStorageVersion(String)}. Parsing is + * case-insensitive. + * + *

The {@code v}-prefixed spellings ("v2_0", "v2.0", "v2_1", "v2.1", "v2_2", "v2.2") are + * deprecated. They were accepted only by this method, never by the rest of Lance, and will be + * removed in a future release — use the numeric version instead ("v2_1" becomes "2.1"). * * @param storageFormat the storage format name * @return this builder instance diff --git a/java/src/test/java/org/lance/CommitBuilderStorageFormatTest.java b/java/src/test/java/org/lance/CommitBuilderStorageFormatTest.java new file mode 100644 index 00000000000..b500369cbb9 --- /dev/null +++ b/java/src/test/java/org/lance/CommitBuilderStorageFormatTest.java @@ -0,0 +1,151 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.lance; + +import org.lance.operation.Append; +import org.lance.operation.Delete; +import org.lance.operation.OperationTestBase; + +import org.apache.arrow.memory.RootAllocator; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.nio.file.Path; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class CommitBuilderStorageFormatTest extends OperationTestBase { + + /** + * Append to a freshly created (2.1) dataset with the given storage format and return the + * committed dataset's format version. + */ + private String commitWithStorageFormat(String datasetPath, String storageFormat) + throws Exception { + try (RootAllocator allocator = new RootAllocator(Long.MAX_VALUE)) { + TestUtils.SimpleTestDataset testDataset = + new TestUtils.SimpleTestDataset(allocator, datasetPath); + dataset = testDataset.createEmptyDataset(); + FragmentMetadata fragment = testDataset.createNewFragment(10); + try (Transaction txn = + new Transaction.Builder() + .readVersion(dataset.version()) + .operation(Append.builder().fragments(Collections.singletonList(fragment)).build()) + .build()) { + try (Dataset committed = + new CommitBuilder(dataset).storageFormat(storageFormat).execute(txn)) { + assertEquals(2, committed.version()); + return committed.getLanceFileFormatVersion(); + } + } + } + } + + /** + * The numeric versions are what {@link Dataset#getLanceFileFormatVersion()} returns and what + * {@link WriteParams.Builder#withDataStorageVersion(String)} accepts, so they must work here too + * — a caller that encodes fragments as "2.1" has to be able to commit them as "2.1". + */ + @Test + void testCanonicalVersionAccepted(@TempDir Path tempDir) throws Exception { + assertEquals( + LanceConstants.FILE_FORMAT_VERSION_2_1, + commitWithStorageFormat( + tempDir.resolve("canonical").toString(), LanceConstants.FILE_FORMAT_VERSION_2_1)); + } + + /** The "v"-prefixed spelling shipped in this method's Javadoc and stays accepted. */ + @Test + void testDeprecatedAliasAccepted(@TempDir Path tempDir) throws Exception { + assertEquals( + LanceConstants.FILE_FORMAT_VERSION_2_1, + commitWithStorageFormat(tempDir.resolve("alias").toString(), "v2_1")); + } + + /** + * A delete adds no data files, so nothing about it depends on the storage format — but {@link + * CommitBuilder#storageFormat(String)} is still validated against the existing dataset for any + * operation other than overwrite. A caller that forwards a configured format on every commit hits + * this on row-level operations against a table written in a different version, so the failure is + * a mismatch error rather than anything to do with parsing. + */ + @Test + void testMismatchedFormatRejectedOnRowLevelOperation(@TempDir Path tempDir) throws Exception { + String datasetPath = tempDir.resolve("mismatch").toString(); + try (RootAllocator allocator = new RootAllocator(Long.MAX_VALUE)) { + TestUtils.SimpleTestDataset testDataset = + new TestUtils.SimpleTestDataset(allocator, datasetPath); + dataset = testDataset.createEmptyDataset(); + FragmentMetadata fragment = testDataset.createNewFragment(10); + try (Transaction appendTxn = + new Transaction.Builder() + .readVersion(dataset.version()) + .operation(Append.builder().fragments(Collections.singletonList(fragment)).build()) + .build()) { + dataset = new CommitBuilder(dataset).execute(appendTxn); + } + assertEquals(LanceConstants.FILE_FORMAT_VERSION_2_1, dataset.getLanceFileFormatVersion()); + + List fragmentIds = + dataset.getFragments().stream() + .map(f -> Long.valueOf(f.getId())) + .collect(Collectors.toList()); + + // "2.2" parses fine, so a failure here is the mismatch guard and not the parser. + try (Transaction deleteTxn = deleteAll(fragmentIds)) { + IllegalArgumentException error = + assertThrows( + IllegalArgumentException.class, + () -> + new CommitBuilder(dataset) + .storageFormat(LanceConstants.FILE_FORMAT_VERSION_2_2) + .execute(deleteTxn)); + assertTrue(error.getMessage().contains("Storage format mismatch"), error.getMessage()); + } + + // The same delete succeeds when the format agrees with the dataset. + try (Transaction deleteTxn = deleteAll(fragmentIds)) { + try (Dataset deleted = + new CommitBuilder(dataset) + .storageFormat(LanceConstants.FILE_FORMAT_VERSION_2_1) + .execute(deleteTxn)) { + assertEquals(0, deleted.countRows()); + } + } + } + } + + private Transaction deleteAll(List fragmentIds) { + return new Transaction.Builder() + .readVersion(dataset.version()) + .operation(Delete.builder().deletedFragmentIds(fragmentIds).predicate("1=1").build()) + .build(); + } + + @Test + void testUnknownFormatRejected(@TempDir Path tempDir) { + assertThrows( + IllegalArgumentException.class, + () -> commitWithStorageFormat(tempDir.resolve("bogus").toString(), "bogus")); + // The alias set is frozen at what shipped, so it does not extend to newer versions. + assertThrows( + IllegalArgumentException.class, + () -> commitWithStorageFormat(tempDir.resolve("v23").toString(), "v2_3")); + } +}