[CLN] schema: build default with config ef & default_knn_index, remove #document population in defaults #5775

jairad26 · 2025-10-30T17:34:21Z

Description of changes

Summarize the changes made by this PR.

Improvements & Bug fixes
- This PR is to clean up how collections are created:
1. when collection configuration was hnsw default and schema was any default, it blindly converted config -> schema. instead, this should use the knn index to build the correct default schema, and only take the embedding function from config
2. when converting config -> schema, it writes #document as the source key for both defaults and #embedding vector indexes. Instead, it should only write #document as the source key for #embedding
3. On the distributed modify path, the json mapping for the boolean type in go does not match the rust type
New functionality
- ...

Test plan

How are these changes tested?
added unit tests for all 8 default cases (config hnsw or spann default, schema hnsw or spann default, default_knn_index), test thta #document does not populate for defaults, and embedding functions do.

[ x] 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?

jairad26 · 2025-10-30T17:34:37Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

github-actions · 2025-10-30T17:34:43Z

sanketkedia · 2025-11-07T00:47:30Z

rust/types/src/collection_schema.rs

+                // for both defaults and #embedding key
+                let mut new_schema = Schema::new_default(default_knn_index);
+
+                if collection_config.embedding_function.is_some() {


Using and_then is more idiomatic rust here

also this should be method instead of inlining here

im not sure what this means

sanketkedia · 2025-11-07T00:57:18Z

rust/types/src/collection_schema.rs

                space: Some(hnsw_config.space.clone()),
                embedding_function: collection_config.embedding_function.clone(),
-                source_key: Some(DOCUMENT_KEY.to_string()), // Default source key
+                source_key: None,


Note to self: will need to migrate existing collections for uniformity

sanketkedia · 2025-11-07T01:02:09Z

rust/types/src/collection_schema.rs

    ) -> Result<Schema, SchemaError> {
        // Start with a default schema structure
-        let mut schema = Schema::new_default(KnnIndex::Spann); // Default to HNSW, will be overridden
+        let mut schema = Schema::new_default(default_knn_index);


why does it matter what default you pass here since it is overridden below by config anyways?

it doesn't but the code feels cleaner like this instead of creating a spann always

sanketkedia · 2025-11-07T01:03:55Z

rust/types/src/collection_schema.rs

        if let Some(embedding_types) = schema.keys.get_mut(EMBEDDING_KEY) {
            if let Some(float_list) = &mut embedding_types.float_list {
                if let Some(vector_index) = &mut float_list.vector_index {
+                    let mut vector_config = vector_config.clone();


don't need to clone vector_config here

sanketkedia · 2025-11-07T01:12:26Z

rust/frontend/src/get_collection_with_segments_provider.rs

-        )
-        .map_err(CollectionsWithSegmentsProviderError::InvalidSchema)?;
-        collection_and_segments_sysdb.collection.schema = Some(reconciled_schema);
+        if collection_and_segments_sysdb.collection.schema.is_none() {


have we tested that schema is None and not {} for older collections?

sanketkedia · 2025-11-07T01:43:08Z

rust/frontend/src/get_collection_with_segments_provider.rs

-        .map_err(CollectionsWithSegmentsProviderError::InvalidSchema)?;
-        collection_and_segments_sysdb.collection.schema = Some(reconciled_schema);
+        if collection_and_segments_sysdb.collection.schema.is_none() {
+            collection_and_segments_sysdb.collection.schema = Some(


If we are always passing schema down to the reader then should we update the reader in distributed_hnsw.rs to use schema instead of collection config (with fallback to legacy metadata). Similar to local_hnsw.rs. Makes things uniform and easier to understand. (I understand that the current code is also correct, this is just a code design nit)

sanketkedia · 2025-11-07T01:48:30Z

rust/segment/src/distributed_spann.rs

-        )
-        .map_err(SpannSegmentWriterError::InvalidSchema)?;
+        let schema = if let Some(schema) = collection.schema.as_ref() {
+            schema.clone()


why clone here?

because i cant do collection.schema directly, i have to borrow the schema with as_ref

the other alternative would be to borrow collection.schema directly. this works, just want to confirm this is safe?

let schema = if let Some(schema) = &collection.schema { schema } else { &Schema::convert_collection_config_to_schema(&collection.config, KnnIndex::Spann) .map_err(SpannSegmentWriterError::InvalidSchema)? };

sanketkedia · 2025-11-07T01:50:55Z

rust/types/src/collection.rs

+    /// The read path needs to tolerate collections that only have a configuration persisted.
+    /// This helper hydrates `schema` from the stored configuration when needed, or regenerates
+    /// the configuration from the existing schema to keep both representations consistent.
+    pub fn reconcile_schema_for_read(&mut self, knn_index: KnnIndex) -> Result<(), SchemaError> {


why read path needs to take a default_knn config. That seems like unnecessary because the knn index type has already been decided and persisted

propel-code-bot · 2025-11-10T17:16:22Z

go/pkg/sysdb/coordinator/model/collection_configuration.go

 	Int          *IntValueType          `json:"int,omitempty"`
 	Float        *FloatValueType        `json:"float,omitempty"`
-	Boolean      *BoolValueType         `json:"boolean,omitempty"`
+	Boolean      *BoolValueType         `json:"bool,omitempty"`


[CriticalError]

The JSON tag has been changed from "boolean" to "bool" in the Go struct and corresponding test JSON strings. However, there's a critical compatibility issue to consider:

Breaking Change Impact: This JSON field name change will break compatibility with existing stored data or API clients that expect the "boolean" field name.

Migration Strategy: If this change is intentional to match Rust's naming convention, you'll need to ensure:

Database migration to rename existing boolean fields to bool

API versioning to handle both field names during transition

Client SDK updates across all languages

If this is purely for Go-Rust JSON compatibility (as mentioned in the PR description), consider:

// Option 1: Support both during transition Boolean *BoolValueType `json:"bool,omitempty" legacy:"boolean,omitempty"` // Option 2: Custom marshaling to handle both names

Could you clarify the migration plan for this breaking change?

Context for Agents

[**CriticalError**] The JSON tag has been changed from `"boolean"` to `"bool"` in the Go struct and corresponding test JSON strings. However, there's a critical compatibility issue to consider: 1. **Breaking Change Impact**: This JSON field name change will break compatibility with existing stored data or API clients that expect the `"boolean"` field name. 2. **Migration Strategy**: If this change is intentional to match Rust's naming convention, you'll need to ensure: - Database migration to rename existing `boolean` fields to `bool` - API versioning to handle both field names during transition - Client SDK updates across all languages If this is purely for Go-Rust JSON compatibility (as mentioned in the PR description), consider: ```go // Option 1: Support both during transition Boolean *BoolValueType `json:"bool,omitempty" legacy:"boolean,omitempty"` // Option 2: Custom marshaling to handle both names ``` Could you clarify the migration plan for this breaking change? File: go/pkg/sysdb/coordinator/model/collection_configuration.go Line: 252

propel-code-bot · 2025-11-10T17:42:10Z

rust/frontend/src/get_collection_with_segments_provider.rs

-        collection_and_segments_sysdb.collection.schema = Some(reconciled_schema);
+        if collection_and_segments_sysdb.collection.schema.is_none() {
+            collection_and_segments_sysdb.collection.schema = Some(
+                Schema::convert_collection_config_to_schema(


[BestPractice]

Critical schema handling gap: When collection.schema is None in the reader path, this code converts config to schema but doesn't persist the result. The conversion is done in-memory only, meaning:

Inconsistent state: The reader gets the converted schema, but the database still has schema = None

Performance impact: Schema conversion happens on every read operation instead of once

Potential race conditions: Multiple readers could be converting the same config simultaneously

Consider adding a mechanism to persist the converted schema back to the database after conversion, or implement a write-through cache to avoid repeated conversions.

// After conversion, consider persisting: if was_none_before_conversion { // Persist the converted schema to avoid future conversions self.persist_schema_if_needed(collection_id, &converted_schema).await?; }

Context for Agents

[**BestPractice**] Critical schema handling gap: When `collection.schema` is `None` in the reader path, this code converts config to schema but doesn't persist the result. The conversion is done in-memory only, meaning: 1. **Inconsistent state**: The reader gets the converted schema, but the database still has `schema = None` 2. **Performance impact**: Schema conversion happens on every read operation instead of once 3. **Potential race conditions**: Multiple readers could be converting the same config simultaneously Consider adding a mechanism to persist the converted schema back to the database after conversion, or implement a write-through cache to avoid repeated conversions. ```rust // After conversion, consider persisting: if was_none_before_conversion { // Persist the converted schema to avoid future conversions self.persist_schema_if_needed(collection_id, &converted_schema).await?; } ``` File: rust/frontend/src/get_collection_with_segments_provider.rs Line: 190

rust/types/src/collection.rs

sanketkedia · 2025-11-12T19:30:06Z

rust/log/src/local_compaction_manager.rs

-            .collection
-            .reconcile_schema_with_config(KnnIndex::Hnsw)?;
+        if !schema_previously_persisted {
+            collection_and_segments.collection.schema = Some(


nit: Minor change - for local we init the schema here if None but for distributed we init in the writer? Would be good to make the behavior uniform across the two

sanketkedia · 2025-11-12T19:43:08Z

rust/types/src/collection.rs


 impl Collection {
    /// Reconcile the collection schema and configuration, ensuring both are consistent.
    pub fn reconcile_schema_with_config(&mut self, knn_index: KnnIndex) -> Result<(), SchemaError> {


Is this method used anywhere now?

I think we can remove this method. Only the in-memory frontend uses it but that should also be changed?

…ent population in defaults

rust/cli/src/commands/vacuum.rs

rust/frontend/src/impls/service_based_frontend.rs

jairad26 force-pushed the jai/fix-default-path-reconcile branch from 4bb47ac to 53142f3 Compare October 30, 2025 17:50

jairad26 changed the title ~~[BUG] schema: build default with config ef & knn_index, remove #document population in defaults~~ [BUG] schema: build default with config ef & default_knn_index, remove #document population in defaults Oct 30, 2025

This comment has been minimized.

Sign in to view

jairad26 force-pushed the jai/fix-default-path-reconcile branch 4 times, most recently from b3975e9 to 83d8fb6 Compare October 30, 2025 19:10

This comment has been minimized.

Sign in to view

jairad26 force-pushed the jai/fix-default-path-reconcile branch 2 times, most recently from b428f20 to 8f2ee14 Compare October 30, 2025 21:37

This comment has been minimized.

Sign in to view

jairad26 force-pushed the jai/fix-default-path-reconcile branch 2 times, most recently from 9ab2efd to 2f1e7bc Compare October 31, 2025 16:59

This comment has been minimized.

Sign in to view

jairad26 force-pushed the jai/fix-default-path-reconcile branch 5 times, most recently from 7ad31cf to dcb63e8 Compare November 1, 2025 01:08

jairad26 changed the base branch from main to graphite-base/5775 November 3, 2025 18:47

jairad26 force-pushed the jai/fix-default-path-reconcile branch from dcb63e8 to 3cd61f8 Compare November 3, 2025 18:47

jairad26 changed the base branch from graphite-base/5775 to jai/fix-is-default-schema-check November 3, 2025 18:47

jairad26 mentioned this pull request Nov 3, 2025

[BUG] is_default_schema does not do a space check for defaults or #embedding #5787

Merged

1 task

jairad26 changed the base branch from jai/fix-is-default-schema-check to graphite-base/5775 November 3, 2025 20:22

jairad26 force-pushed the jai/fix-default-path-reconcile branch from 3cd61f8 to 1c0b019 Compare November 3, 2025 20:22

jairad26 force-pushed the graphite-base/5775 branch from 8ad206c to abba8f9 Compare November 3, 2025 20:22

jairad26 changed the base branch from graphite-base/5775 to main November 3, 2025 20:23

jairad26 force-pushed the jai/fix-default-path-reconcile branch from 1c0b019 to 463f83b Compare November 3, 2025 20:52

jairad26 mentioned this pull request Nov 6, 2025

[TST] add schema proptest to python #5823

Open

1 task

jairad26 force-pushed the jai/fix-default-path-reconcile branch from 061ee06 to fe04c8f Compare November 6, 2025 18:00

sanketkedia reviewed Nov 7, 2025

View reviewed changes

jairad26 force-pushed the jai/fix-default-path-reconcile branch from fe04c8f to db3b74e Compare November 7, 2025 19:26

This comment has been minimized.

Sign in to view

jairad26 force-pushed the jai/fix-default-path-reconcile branch from db3b74e to f8cfb53 Compare November 10, 2025 17:10

propel-code-bot bot reviewed Nov 10, 2025

View reviewed changes

jairad26 force-pushed the jai/fix-default-path-reconcile branch from f8cfb53 to 2b76659 Compare November 10, 2025 17:37

propel-code-bot bot reviewed Nov 10, 2025

View reviewed changes

jairad26 force-pushed the jai/fix-default-path-reconcile branch from 2b76659 to e0d238f Compare November 10, 2025 18:04

sanketkedia reviewed Nov 10, 2025

View reviewed changes

rust/types/src/collection.rs Outdated Show resolved Hide resolved

jairad26 force-pushed the jai/fix-default-path-reconcile branch 2 times, most recently from 5fafa38 to 7c682b1 Compare November 10, 2025 23:47

jairad26 mentioned this pull request Nov 11, 2025

[TST] add proptest for config & schema reconciliation #5847

Open

1 task

sanketkedia reviewed Nov 12, 2025

View reviewed changes

sanketkedia approved these changes Nov 12, 2025

View reviewed changes

jairad26 force-pushed the jai/fix-default-path-reconcile branch from 7c682b1 to 8671ea1 Compare November 12, 2025 23:56

[BUG] schema: build default with config ef & knn_index, remove #docum…

c129192

…ent population in defaults

jairad26 force-pushed the jai/fix-default-path-reconcile branch from 8671ea1 to c129192 Compare November 13, 2025 00:15

propel-code-bot bot reviewed Nov 13, 2025

View reviewed changes

rust/cli/src/commands/vacuum.rs Show resolved Hide resolved

propel-code-bot bot reviewed Nov 13, 2025

View reviewed changes

rust/frontend/src/impls/service_based_frontend.rs Show resolved Hide resolved

jairad26 merged commit 3d9843b into main Nov 13, 2025
62 checks passed

[CLN] schema: build default with config ef & default_knn_index, remove #document population in defaults #5775

[CLN] schema: build default with config ef & default_knn_index, remove #document population in defaults #5775

Uh oh!

Conversation

jairad26 commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Test plan

Migration plan

Observability plan

Documentation Changes

Uh oh!

jairad26 commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 30, 2025

Reviewer Checklist

Testing, Bugs, Errors, Logs, Documentation

System Compatibility

Quality

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanketkedia Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jairad26 Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

propel-code-bot bot Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

propel-code-bot bot Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jairad26 commented Oct 30, 2025 •

edited

Loading

jairad26 commented Oct 30, 2025 •

edited

Loading

sanketkedia Nov 7, 2025 •

edited

Loading

jairad26 Nov 7, 2025 •

edited

Loading