fix: skip object store check when UseRESTCatalogCommit is enabled#328
Open
liujiayi771 wants to merge 3 commits into
Open
fix: skip object store check when UseRESTCatalogCommit is enabled#328liujiayi771 wants to merge 3 commits into
liujiayi771 wants to merge 3 commits into
Conversation
|
|
liujiayi771
force-pushed
the
fix/skip-object-store-check-for-rest-commit
branch
from
0ece315 to
2f5ec3b
Compare
When UseRESTCatalogCommit(true) is set, FileStoreCommit does not write snapshot files to the filesystem — it delegates the final commit to the REST catalog server. The object store restriction (which exists because atomic rename is not supported on object stores) does not apply in this mode since no snapshot file needs to be atomically written. This allows external catalog integrations (e.g., DLF) to use FileStoreCommit with oss:// table paths without needing the "enable-object-store-commit-in-inte-test" workaround flag. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
liujiayi771
force-pushed
the
fix/skip-object-store-check-for-rest-commit
branch
from
2f5ec3b to
d9791b1
Compare
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
3 participants
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.
Purpose
When
UseRESTCatalogCommit(true)is set,FileStoreCommitdelegates the final snapshot commit to the REST catalog server instead of writing the snapshot file to the filesystem. The existing object store restriction (which blocks commit onoss://,s3://, etc.) exists because atomic rename is not supported on object stores — but this restriction does not apply in REST commit mode since no snapshot file needs to be atomically written by the client.Currently, external catalog integrations that use
UseRESTCatalogCommit(true)with object store table paths are forced to set the internal test flagenable-object-store-commit-in-inte-testto bypass this check. This PR relaxes the constraint so thatUseRESTCatalogCommit(true)alone is sufficient.Changes
file_store_commit.cpp: Added!ctx->UseRESTCatalogCommit()to the object store check condition, so REST commit mode skips the restriction.Tests
TestObjectStoreBlockedWithoutRESTCommit: Verifies that non-REST commit mode still correctly identifies object store vs. local paths viaFileSystem::IsObjectStore().TestRESTCatalogCommitWithObjectStorePath: Verifies that REST commit mode works end-to-end (Create → Commit → GetLastCommitTableRequest) and does not write snapshot files to the filesystem.All 30 existing
FileStoreCommitImplTesttests continue to pass.