fix(jni): align CommitBuilder.storageFormat() parsing with LanceFileVersion::from_str - #8063
fix(jni): align CommitBuilder.storageFormat() parsing with LanceFileVersion::from_str#8063ivscheianu wants to merge 6 commits into
Conversation
0ecde63 to
d66de1c
Compare
…ersion::from_str
The JNI `parse_storage_format` function (used by `CommitBuilder.storageFormat()`)
had a hand-rolled match that accepted only a subset of format strings ("v2.1",
"v2_1", etc.) while the fragment creation path (`extract_write_params`) uses
`LanceFileVersion::from_str` which also accepts the canonical numeric forms
("2.1", "2.2").
This inconsistency causes `CommitBuilder.storageFormat("2.1")` to fail with
"Unknown storage format: 2.1" even though the same string works correctly
in `WriteParams.dataStorageVersion`.
Replace the custom match with `name.parse::<LanceFileVersion>()` and extend
`FromStr` to also accept the prefixed aliases ("v2_0", "v2.0", etc.) so no
previously accepted values are rejected.
2ff1fd3 to
a1ba7f8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
There was a problem hiding this comment.
The change fixes the parser drift at the shared Rust boundary while preserving all previously accepted CommitBuilder spellings. Canonical and compatibility forms resolve to the same versions, invalid inputs retain contextual errors, and manifest decoding remains restricted to canonical exact versions. I found no correctness or compatibility issue that should block it.
The "v"-prefixed storage format spellings ("v2_1", "v2.1") were never a
Lance concept. They were introduced in lance-format#5978, where `parse_storage_format`
hand-rolled its match by walking the `LanceFileVersion` variant identifiers
(`V2_1` -> "v2_1") rather than delegating to `FromStr`. That is why the JNI
accepted the identifiers and rejected the canonical "2.1" that `Display`
emits and every other binding accepts.
Rather than teach core's `FromStr` the aliases, keep them where they came
from. `parse_storage_format` now translates the alias set to its canonical
form, warns that the spelling is deprecated, and delegates to `FromStr`, so
core, Python, and `extract_write_params` keep a single unchanged contract.
The alias set is frozen at what shipped in the `CommitBuilder.storageFormat`
Javadoc from 3.0.0 ("v2_0", "v2_1", "v2_2"); it deliberately does not extend
to 2.3 or later, which are reachable only by their canonical name.
This reverts the `lance-encoding` and `lance-file` changes from the previous
commit on this branch, leaving the fix entirely inside the Java binding.
Tests: `parse_storage_format` unit tests for canonical forms, deprecated
aliases, case-insensitivity, invalid input, and the frozen alias set, plus
`CommitBuilderStorageFormatTest` covering the end-to-end Java path.
That Java test also pins a constraint downstream callers hit: `storageFormat`
is validated against the existing dataset for any non-overwrite operation, so
a delete that adds no data files still fails when the format disagrees.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hamersaw
left a comment
hamersaw
left a comment
There was a problem hiding this comment.
Thanks for the fix! I submitting a PR into this at https://github.com/ivscheianu/lance/pull/1/changes that updates this slightly. I don't think we should be updating the base format to respect the "v2_1", "v2_2", etc notation. I see this was added to the JNI implementation in a PR I submitting awhile back. I think that was erroneous and we should do everything to encapsulate that in the JNI. I think with my proposal the spark-side for this fix will still work. Please take a look and let me know what you think.
fix(jni): keep storage format aliases in the JNI instead of core
Thank you, @hamersaw! Merged the PR 👍🏻 |
There was a problem hiding this comment.
Gate recommendation: approve. This keeps the Java compatibility aliases at the JNI boundary while delegating canonical version parsing to the shared Rust implementation. Both dataset and URI commit paths use the same parser, and focused Rust and Java integration tests cover the accepted spellings, invalid input, and propagation into the dataset builder.
2 participants
Fixes #8066
The JNI
parse_storage_format(used byCommitBuilder.storageFormat()) had a hand-rolled match that only accepted prefixed aliases ("v2_1","v2.1") whileextract_write_paramsusesLanceFileVersion::from_strwhich accepts the canonical numeric forms ("2.1","2.2"). This surfaced when lance-spark#730 started propagatingfile_format_versiontoCommitBuilder.storageFormat(), breaking some tests.Fix:
name.parse::<LanceFileVersion>().FromStrto also accept the prefixed aliases so no previously valid input is rejected.CommitBuilder.storageFormat()Javadoc.