test: cover multi-base reads with shared-memory stores - #8824
Open
zhangyue19921010 wants to merge 4 commits into
Open
test: cover multi-base reads with shared-memory stores#8824zhangyue19921010 wants to merge 4 commits into
zhangyue19921010 wants to merge 4 commits into
Conversation
Multi-base tests were built on `memory://`, whose bytes do not outlive a single `ObjectStore` instance: a write resolves its base stores through a registry of its own when no `Session` is supplied, while reads resolve them through the dataset's session. Data written into a base was therefore unreadable, so those tests could only assert manifest metadata and never exercised the multi-base read path. Move them to `shared-memory://`, whose backends are process-global and keyed by authority, and scan the data back where it was only counted before. `test_multi_base_create` reads its base fragment through both the writer's handle and a freshly opened dataset; `test_multi_base_target_primary_and_bases` scans the reopened dataset so its 21 rows actually come off primary, base1 (a dataset root) and base2 (not one) instead of `count_rows` answering from the manifest. Give each test its own authority so the process-global pool stays isolated; this also removes the `bucket1`/`bucket2` names four `add_bases` tests shared. Document the constraint on `MemoryStoreProvider` so the next multi-base test does not repeat it.
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
the
K-approved
lance-gatekeeper
Bot
removed
the
K-approved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
the
K-approved
lance-gatekeeper
Bot
removed
the
K-approved
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The merged base leaves the author patch unchanged. Shared-memory provider, registry, and fresh-session semantics remain intact, and reopened scans continue to validate exact rows across primary and registered base paths.
Keeping memory stores instance-local preserves their isolation contract; limiting shared memory to these multi-actor fixtures remains the cleaner solution.
lance-gatekeeper
Bot
added
the
K-approved
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.
Multi-base tests were built on
memory://, whose bytes do not outlive a singleObjectStoreinstance: a write resolves its base stores through a registry of its own when noSessionis supplied, while reads resolve them through the dataset's session. Data written into a base was therefore unreadable, so those tests could only assert manifest metadata and never exercised the multi-base read path.Move them to
shared-memory://, whose backends are process-global and keyed by authority, and assert intest_multi_base_createthat the fragment written into a base reads back both through the writer's handle and through a freshly opened dataset. Both assertions fail onmemory://.Give each test its own authority so the process-global pool stays isolated; this also removes the
bucket1/bucket2names fouradd_basestests shared. Document the constraint onMemoryStoreProviderso the next multi-base test does not repeat it.