Skip to content

[BUG](sysdb): preserve legacy hnsw: metadata keys - #6953

Merged
rescrv merged 1 commit into
mainfrom
rescrv/decorations2
Apr 28, 2026
Merged

[BUG](sysdb): preserve legacy hnsw: metadata keys#6953
rescrv merged 1 commit into
mainfrom
rescrv/decorations2

Conversation

@rescrv

@rescrv rescrv commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Description of changes

Legacy "hnsw:" prefixed keys are user-visible collection metadata
and must round-trip through CreateCollectionRequest. The previous
code incorrectly filtered them out during conversion from proto.

  • Remove metadata.retain() call that stripped hnsw: keys
  • Add unit test verifying hnsw:space survives round-trip
  • Annotate schema property tests with @multi_region_test

Test plan

CI

Migration plan

N/A

Observability plan

N/A

Documentation Changes

N/A

Co-authored-by: AI

@propel-code-bot

propel-code-bot Bot commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Fix CreateCollectionRequest metadata conversion to preserve legacy hnsw: keys

This PR fixes a regression in rust/rust-sysdb/src/types.rs where legacy metadata keys prefixed with hnsw: were being removed during protobuf-to-internal conversion for CreateCollectionRequest. The filtering logic (metadata.retain(...)) was removed so these user-visible keys now round-trip correctly.

It also adds a targeted unit test that builds a chroma_proto::CreateCollectionRequest with hnsw:space metadata and verifies it is preserved as MetadataValue::Str("cosine") after conversion. In Python property tests, two schema-related tests in chromadb/test/property/test_schema.py were annotated with @multi_region_test, with corresponding import updates.

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

@github-actions

Copy link
Copy Markdown

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 propel-code-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review found no issues; the regression fix and added test coverage appear sound.

Status: No Issues Found | Risk: Low

Review Details

📁 2 files reviewed | 💬 0 comments

@rescrv
rescrv requested a review from tanujnay112 April 23, 2026 15:56
Base automatically changed from rescrv/decorations to main April 27, 2026 16:55
@rescrv
rescrv force-pushed the rescrv/decorations2 branch from aae6dd8 to 378e1cd Compare April 27, 2026 16:56
Legacy "hnsw:" prefixed keys are user-visible collection metadata
and must round-trip through CreateCollectionRequest. The previous
code incorrectly filtered them out during conversion from proto.

- Remove metadata.retain() call that stripped hnsw: keys
- Add unit test verifying hnsw:space survives round-trip
- Annotate schema property tests with @multi_region_test

Co-authored-by: AI
@rescrv
rescrv force-pushed the rescrv/decorations2 branch from 378e1cd to a2213d6 Compare April 27, 2026 23:30
@blacksmith-sh

This comment has been minimized.

@rescrv
rescrv merged commit d9b79e3 into main Apr 28, 2026
121 of 123 checks passed
@rescrv
rescrv deleted the rescrv/decorations2 branch April 28, 2026 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants