fix: merge write-time storage options into staged commits#714
Open
ivscheianu wants to merge 2 commits into
Open
fix: merge write-time storage options into staged commits#714ivscheianu wants to merge 2 commits into
ivscheianu wants to merge 2 commits into
Conversation
Fixes lance-format#712. StagedCommit captures storage options once at stage-create time from the catalog's static config. For path-based staged creates/replaces (no namespace), that is the only storage-options source available at that point, so write-time `.option(...)` values and namespace-vended credentials obtained afterward were silently dropped from the final manifest commit, falling back to ambient credential discovery instead. LanceBatchWrite.commit() now merges writeOptions.getStorageOptions() and initialStorageOptions into the StagedCommit before it commits, mirroring the merge the non-staged branch already performs and the merge LanceSparkWriteOptions.toWriteParams() already performs for executor-side fragment writes. StagedCommit also now defensively copies its storage options map at construction time, since callers (e.g. the catalog's static config) may hand it an unmodifiable map.
Some comments overstated when StagedCommit only has the catalog's
static options (true for path-based staged creates, not namespace-based
ones), referenced a commit call that doesn't happen in that method, and
used a term ("ambient credential discovery") that appears nowhere else
in the codebase. Others named specific call sites that don't actually
exercise the scenario they claimed to simulate. Trimmed to state only
what's accurate and load-bearing.
Contributor
Author
|
Hello @hamersaw, small one. I'd appreciate it if you could have a look when you get a chance. Thanks a lot! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are included in this PR?
Fixes #712.
StagedCommitcaptures storage options once at stage-create time from the catalog's static config. For path-based staged creates/replaces (no namespace), that is the only storage-options source available at that point, so write-time.option(...)values and namespace-vended credentials obtained afterward were silently dropped from the final manifest commit, falling back to ambient credential discovery instead.This PR:
StagedCommit.mergeStorageOptions(Map<String, String>)to layer in additional options after construction.LanceBatchWrite.commit()'s staged branch withLanceRuntime.mergeStorageOptions(writeOptions.getStorageOptions(), initialStorageOptions)— the same merge and precedence the non-staged branch andLanceSparkWriteOptions.toWriteParams()(executor-side fragment writes) already use.StagedCommit's constructor defensively copy the incoming storage options map, since callers (e.g.LanceSparkCatalogConfig.getStorageOptions()) can hand it an unmodifiable map, which would otherwise make the new merge throwUnsupportedOperationExceptionon every real path-based staged create.All six staged entry points (
stageCreate[AtPath],stageReplace[AtPath],stageCreateOrReplace[AtPath]) funnel through this singleLanceBatchWrite.commit()staged branch, so the fix closes the gap uniformly without touching call sites individually.AddColumnsBackfillBatchWrite/UpdateColumnsBackfillBatchWrite/SparkPositionDeltaWriteare unaffected — they never useStagedCommitand already perform their own correct merge.Are these changes tested?
StagedCommitTest:mergeStorageOptionsadds new keys, overrides existing keys on conflict, is a no-op for null/empty input, does not mutate the caller's map, and accepts an unmodifiable base map (regression test for theUnsupportedOperationExceptionrisk above).LanceBatchWriteTest:LanceBatchWrite.commit()mergesinitialStorageOptionsinto aStagedCommitthat started with empty options (simulatingstageCreateAtPath), including a full round-trip through an actual dataset commit; and a separate test confirming write-time-only options survive the merge whileinitialStorageOptionsstill wins on key conflict, matchingLanceRuntime.mergeStorageOptions's documented precedence../mvnw test -pl lance-spark-3.5_2.12 -am— 1238 tests, 0 failures, 0 errors.