Skip to content

[HOTFIX] applying PR #5418 to rc/2025-09-05 #5421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 5, 2025
Merged

[HOTFIX] applying PR #5418 to rc/2025-09-05 #5421

merged 1 commit into from
Sep 5, 2025

Conversation

@chroma-droid
Copy link

This PR cherry-picks the commit 17e083a onto rc/2025-09-05. If there are unresolved conflicts, please resolve them manually.

@Sicheng-Pan @github-actions
[BUG] Sparse vector index incorrect blockfile path (#5418)
d3c7142 
## Description of changes

_Summarize the changes made by this PR._

- Improvements & Bug fixes
  - After collection fork, the sparse vector blockfile are put under the child collection prefix, which is inconsistent to the existing behavior where new blockfiles are put under the root collection prefix. Although the long term fix is to move all new blockfiles after fork under child collection prefix, as a temporary workaround we just put the sparse vector blockfiles under the root collection as well. 
- New functionality
  - N/A

## Test plan

_How are these changes tested?_

- [ ] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust

## Migration plan

_Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?_

## Observability plan

_What is the plan to instrument and monitor this change?_

## Documentation Changes

_Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the [docs section](https://github.com/chroma-core/chroma/tree/main/docs/docs.trychroma.com)?_
@github-actions
Copy link

github-actions bot commented Sep 5, 2025

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@propel-code-bot
Copy link
Contributor

Cherry-Pick: Fix Sparse Vector Index Blockfile Path on Collection Fork

This hotfix cherry-picks PR #5418 into the rc/2025-09-05 branch. The update ensures that after a collection fork, newly created sparse vector blockfiles are consistently placed under the root collection's prefix instead of the forked collection's prefix, maintaining path consistency with other blockfile types. The fix is described as a temporary workaround; the long-term design would move all new blockfiles to the forked collection's prefix. The change also includes a comprehensive test verifying prefix correctness and backward compatibility with legacy and missing file scenarios.

Key Changes

• Adjusts logic in MetadataSegmentWriter::from_segment to set the prefix of new sparse vector blockfiles to match the prefix of any existing blockfiles in the segment.
• Adds detailed comments in the affected Rust function clarifying the reasoning and temporary status of the fix.
• Introduces a new async test (test_sparse_index_recreated_with_existing_prefix) to validate that sparse index files are recreated under the correct prefix, even during fork and legacy scenarios.
• Expands the test suite by 239 lines to cover edge-cases, regressions and legacy situations related to blockfile path prefixing.

Affected Areas

• rust/segment/src/blockfile_metadata.rs
• MetadataSegmentWriter::from_segment
• Unit/integration tests for blockfile_metadata

This summary was automatically generated by @propel-code-bot

propel-code-bot[bot]
propel-code-bot bot reviewed Sep 5, 2025
View reviewed changes
@Sicheng-Pan Sicheng-Pan merged commit 6de9b6a into rc/2025-09-05 Sep 5, 2025
59 checks passed
@Sicheng-Pan Sicheng-Pan deleted the hotfix-5418/rc/2025-09-05 branch September 5, 2025 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@propel-code-bot propel-code-bot[bot] propel-code-bot[bot] left review comments

@Sicheng-Pan Sicheng-Pan Sicheng-Pan approved these changes

Assignees

@Sicheng-Pan Sicheng-Pan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chroma-droid @Sicheng-Pan