fix(jni): align CommitBuilder.storageFormat() parsing with LanceFileVersion::from_str

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<LanceFileVersion> {
-    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::<LanceFileVersion>()
+        .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());
+    }
 }

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.
    *
    * <p>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.
+   *
+   * <p>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

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<Long> 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<Long> 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"));
+  }
+}