Use predefined deferred ID#8329
Merged
Merged
Conversation
IvanPleshkov
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/segment/src/segment_constructor/segment_constructor_base.rs`:
- Around line 654-656: Update the documentation comment for the field
deferred_internal_id to reflect the new invariant: when segment.is_appendable()
the field stores the configured threshold immediately upon construction (it is
not necessarily None), and points are considered deferred only when their
internal ID is >= deferred_internal_id; rely on the accessors
has_deferred_points() and point_is_deferred() to check deferred status rather
than assuming None means “no deferred points.” Mention deferred_internal_id,
has_deferred_points(), point_is_deferred(), and segment.is_appendable() in the
comment so readers know where to check the runtime semantics.
In `@src/actix/api/snapshot_api.rs`:
- Line 38: Split the combined import statement `use {actix_web_validator as
valid, fs_err as fs};` into two separate `use` lines (one for
`actix_web_validator as valid` and one for `fs_err as fs`) to match the file's
existing import style and resolve the rustfmt failure; update the import section
where the combined statement appears (referencing the symbol `valid` and `fs`)
so each crate is imported on its own `use` statement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 414c4254-bb59-4387-9d99-7304bc44a380
📒 Files selected for processing (23)
lib/collection/src/collection_manager/fixtures.rslib/collection/src/collection_manager/optimizers/config_mismatch_optimizer.rslib/collection/src/collection_manager/optimizers/indexing_optimizer.rslib/collection/src/collection_manager/optimizers/merge_optimizer.rslib/collection/src/collection_manager/optimizers/vacuum_optimizer.rslib/collection/src/config.rslib/collection/src/optimizers_builder.rslib/collection/src/shards/local_shard/mod.rslib/collection/src/shards/local_shard/snapshot.rslib/collection/src/tests/mod.rslib/collection/src/update_workers/optimization_worker.rslib/edge/src/optimize.rslib/segment/src/segment/mod.rslib/segment/src/segment/segment_ops.rslib/segment/src/segment/tests.rslib/segment/src/segment_constructor/segment_builder.rslib/segment/src/segment_constructor/segment_constructor_base.rslib/shard/src/operations/optimization.rslib/shard/src/optimize.rslib/shard/src/optimizers/segment_optimizer.rslib/shard/src/segment_holder/mod.rslib/shard/src/segment_holder/snapshot.rssrc/actix/api/snapshot_api.rs
💤 Files with no reviewable changes (1)
- lib/segment/src/segment/mod.rs
IvanPleshkov
force-pushed
the
use-predefined-deferred-internal-id
branch
from
d6a599f to
705b35c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/collection/src/config.rs`:
- Around line 230-246: The computed value threshold_bytes.div_ceil(vector_bytes)
can exceed PointOffsetType (u32) and must be clamped or checked before the
unchecked `as PointOffsetType` cast in the closure; change the cast to a checked
conversion (e.g. usize::try_into with handling) or saturate by taking min(value,
PointOffsetType::MAX as usize) and then cast, referencing the closure that
computes element_bytes (Datatype::Float16/Uint8/Float32), dim (params.size),
MULTIVECTOR_SIZE and params.multivector_config so you clamp/handle overflow of
the result of threshold_bytes.div_ceil(vector_bytes) instead of using `as
PointOffsetType`.
- Around line 221-246: The size calculation here uses raw datatype * dim and
ignores vector/collection-level quantization, causing deferred_internal_id to be
undersized; update the sizing logic in this iterator (the closure that computes
vector_bytes) to use the same effective vector data inputs as used by
CollectionParams::to_base_vector_data() (i.e., resolve effective
per-vector/collection quantization and multivector layout before computing
element size and vector length) so element_bytes and vector multiplier match
appendable segment construction; either call or replicate the same computation
from CollectionParams::to_base_vector_data() when deriving vector_bytes
(respecting params.multivector_config and collection-level
appendable_quantization) so threshold_bytes.div_ceil(...) uses the correct
effective byte size.
In `@lib/segment/src/segment_constructor/segment_constructor_base.rs`:
- Line 32: Remove the unused import NonAppendableSegmentEntry from
segment_constructor_base.rs: locate the use statement referencing
NonAppendableSegmentEntry and delete it so the file no longer imports that
unused symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 639181f8-d208-46a6-a924-629d4e6ebd81
📒 Files selected for processing (22)
lib/collection/src/collection_manager/fixtures.rslib/collection/src/collection_manager/optimizers/config_mismatch_optimizer.rslib/collection/src/collection_manager/optimizers/indexing_optimizer.rslib/collection/src/collection_manager/optimizers/merge_optimizer.rslib/collection/src/collection_manager/optimizers/vacuum_optimizer.rslib/collection/src/config.rslib/collection/src/optimizers_builder.rslib/collection/src/shards/local_shard/mod.rslib/collection/src/shards/local_shard/snapshot.rslib/collection/src/tests/mod.rslib/collection/src/update_workers/optimization_worker.rslib/edge/src/optimize.rslib/segment/src/segment/mod.rslib/segment/src/segment/segment_ops.rslib/segment/src/segment/tests.rslib/segment/src/segment_constructor/segment_builder.rslib/segment/src/segment_constructor/segment_constructor_base.rslib/shard/src/operations/optimization.rslib/shard/src/optimize.rslib/shard/src/optimizers/segment_optimizer.rslib/shard/src/segment_holder/mod.rslib/shard/src/segment_holder/snapshot.rs
💤 Files with no reviewable changes (1)
- lib/segment/src/segment/mod.rs
✅ Files skipped from review due to trivial changes (1)
- lib/segment/src/segment_constructor/segment_builder.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- lib/shard/src/operations/optimization.rs
- lib/shard/src/optimizers/segment_optimizer.rs
- lib/collection/src/collection_manager/optimizers/indexing_optimizer.rs
- lib/collection/src/update_workers/optimization_worker.rs
- lib/segment/src/segment/tests.rs
- lib/edge/src/optimize.rs
- lib/collection/src/tests/mod.rs
- lib/collection/src/collection_manager/optimizers/vacuum_optimizer.rs
- lib/collection/src/shards/local_shard/snapshot.rs
IvanPleshkov
force-pushed
the
use-predefined-deferred-internal-id
branch
from
771921a to
4e063cf
Compare
IvanPleshkov
changed the base branch from
dev
to
update-delete-points-logic-on-deferred
IvanPleshkov
force-pushed
the
use-predefined-deferred-internal-id
branch
from
97d5c50 to
69b2082
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
lib/segment/src/segment/tests.rs (1)
780-823: Prefer the normal upsert path in this persistence test.Lines 780-823 currently use
insert_new_vectors()and then patchsegment.versionon Line 823. That bypasses the usual point-version bookkeeping, so the flushed segment does not match a production-written one. Usingupsert_point()here would cover the same deferred-ID behavior without manual state mutation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/segment/src/segment/tests.rs` around lines 780 - 823, The test currently calls insert_new_vectors in a loop and then mutates segment.version, which bypasses real per-point version bookkeeping; replace the insert_new_vectors(...) calls with calls to upsert_point(...) (passing the same PointIdType, payload/vector via only_default_vector, and hw_counter) so versions are set via the normal upsert path and the deferred-ID behavior is exercised naturally, and remove the manual assignment to segment.version (line setting segment.version = Some(20)); keep the remainder of the assertions (has_deferred_points, deferred_internal_id, point_is_deferred checks) unchanged.lib/shard/src/fixtures.rs (1)
164-196: Align this fixture with the new HNSW-backed deferred path.
deferred_internal_idis now derived from HNSW-configured vectors, but this helper still builds a plain-only segment and asserts deferred behavior anyway. That makes the fixture less representative of the production path. I’d either switch the vector config here to HNSW or rename/document this as an intentionally artificial segment state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/shard/src/fixtures.rs` around lines 164 - 196, The fixture build_segment_with_deferred_1 currently constructs a plain-only segment (Indexes::Plain) but uses a deferred_internal_id that in production is derived from HNSW-configured vectors; update the fixture to reflect the production path by switching the vector index to an HNSW index (replace Indexes::Plain with the appropriate Indexes::Hnsw configuration in the SegmentConfig/VectorDataConfig for DEFAULT_VECTOR_NAME and ensure any required HNSW config fields are provided), or if you intentionally want an artificial plain-only state, rename the helper and its docstring to explicitly state it’s a synthetic test case that does not reflect HNSW-derived deferred IDs (update function name build_segment_with_deferred_1 and its comment accordingly) so the intent is clear.lib/collection/src/tests/mod.rs (1)
241-246: Add a non-Noneregression case for deferred-ID propagation.This setup still passes if
ensure_appendable_segment_with_capacity()stops forwardingdeferred_internal_id, because the test never exercises that path. A small follow-up that creates the appendable segment withSome(...)and asserts deferred behavior after a few inserts would lock the new wiring down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/collection/src/tests/mod.rs` around lines 241 - 246, The test uses OptimizerThresholds with deferred_internal_id = None so it doesn't exercise deferred-ID propagation; add a second regression case that sets OptimizerThresholds.deferred_internal_id = Some(<unique id>) before calling ensure_appendable_segment_with_capacity (the same helper used in the test) then perform a few inserts (the existing insert loop) and assert that the created appendable segment (inspect the segment returned by ensure_appendable_segment_with_capacity or the collection's active segment) carries/propagates the deferred_internal_id as expected (e.g., segment.internal_id or deferred marker) to lock down deferred-ID wiring.lib/segment/src/segment_constructor/segment_constructor_base.rs (1)
781-785: Document the newdeferred_internal_idparameter on the public constructors.
load_segment()andbuild_segment()now expose this parameter publicly, but the rustdoc still doesn't explain what it represents or that it is only honored for appendable segments. Adding that here would save callers from having to reverse-engineer the invariant fromcreate_segment().Also applies to: 865-869
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/segment/src/segment_constructor/segment_constructor_base.rs` around lines 781 - 785, Update the public rustdoc for the constructors (notably load_segment and build_segment) to document the new deferred_internal_id parameter: explain what deferred_internal_id represents (the next internal point offset to be used), that it is only honored for appendable segments, and any invariants/cases callers must observe (e.g., should be None for non-appendable segments or when creating from scratch). Also mirror the same documentation change where build_segment and create_segment are documented so callers can find the behavior consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/collection/src/config.rs`:
- Around line 224-229: The current filter uses the merged effective_hnsw to
decide if a vector has HNSW, which incorrectly counts vectors that explicitly
disabled HNSW via a per-vector override (e.g., params.hnsw_config with m = 0)
when collection-level payload_m is non-zero; change the predicate to respect
explicit per-vector overrides: determine HNSW-enabled by checking
params.hnsw_config first (if Some, use that config's m or payload_m to decide
enabled) and only if None fall back to the collection-level hnsw_config values;
update the closure around update_opt / effective_hnsw to use this
per-vector-first predicate so deferred_internal_id is computed correctly for
mixed indexed/non-indexed collections.
In `@lib/collection/src/optimizers_builder.rs`:
- Around line 266-272: threshold_config is computed using a single
collection-level num_indexing_threads which ignores per-vector HNSW overrides;
change the call to optimizers_config.optimizer_thresholds so
num_indexing_threads is first derived by iterating the effective per-vector HNSW
configs (resolve each config's max_indexing_threads where 0 is treated as the
fallback/default thread count) and taking the max() across them, then pass that
resolved max into optimizer_thresholds; update references around
collection_params.get_deferred_point_id, hnsw_config, and
optimizers_config.get_deferred_points_threshold_bytes() accordingly so
threshold_config uses the per-vector-resolved thread count.
In `@lib/segment/src/segment/mod.rs`:
- Around line 97-99: Fix the typo in the doc comment for the struct field
deferred_internal_id: change "Avaliable for appendable segments only." to
"Available for appendable segments only." so Codespell passes; the change should
be in the doc string immediately above pub(crate) deferred_internal_id:
Option<PointOffsetType>.
In `@lib/segment/src/segment/segment_ops.rs`:
- Around line 656-660: has_deferred_points() currently compares
deferred_internal_id against total_point_count(), which is the count of assigned
internal IDs and stays > deferred_from even after those points are deleted;
change the check to derive from live mappings instead. Update the body of
has_deferred_points() (referencing deferred_internal_id, is_appendable(),
total_point_count()) to compute whether any non-deleted internal ID >=
deferred_from by scanning the live mapping structure (e.g., the
internal->external mapping or use the
highest_non_deleted_internal_id()/highest_live_internal_id() helper if
available) and return self.is_appendable() &&
(there_exists_live_internal_id_geq_deferred_from). Ensure the scan uses the
canonical live-mapping data structure so deletions are respected.
---
Nitpick comments:
In `@lib/collection/src/tests/mod.rs`:
- Around line 241-246: The test uses OptimizerThresholds with
deferred_internal_id = None so it doesn't exercise deferred-ID propagation; add
a second regression case that sets OptimizerThresholds.deferred_internal_id =
Some(<unique id>) before calling ensure_appendable_segment_with_capacity (the
same helper used in the test) then perform a few inserts (the existing insert
loop) and assert that the created appendable segment (inspect the segment
returned by ensure_appendable_segment_with_capacity or the collection's active
segment) carries/propagates the deferred_internal_id as expected (e.g.,
segment.internal_id or deferred marker) to lock down deferred-ID wiring.
In `@lib/segment/src/segment_constructor/segment_constructor_base.rs`:
- Around line 781-785: Update the public rustdoc for the constructors (notably
load_segment and build_segment) to document the new deferred_internal_id
parameter: explain what deferred_internal_id represents (the next internal point
offset to be used), that it is only honored for appendable segments, and any
invariants/cases callers must observe (e.g., should be None for non-appendable
segments or when creating from scratch). Also mirror the same documentation
change where build_segment and create_segment are documented so callers can find
the behavior consistently.
In `@lib/segment/src/segment/tests.rs`:
- Around line 780-823: The test currently calls insert_new_vectors in a loop and
then mutates segment.version, which bypasses real per-point version bookkeeping;
replace the insert_new_vectors(...) calls with calls to upsert_point(...)
(passing the same PointIdType, payload/vector via only_default_vector, and
hw_counter) so versions are set via the normal upsert path and the deferred-ID
behavior is exercised naturally, and remove the manual assignment to
segment.version (line setting segment.version = Some(20)); keep the remainder of
the assertions (has_deferred_points, deferred_internal_id, point_is_deferred
checks) unchanged.
In `@lib/shard/src/fixtures.rs`:
- Around line 164-196: The fixture build_segment_with_deferred_1 currently
constructs a plain-only segment (Indexes::Plain) but uses a deferred_internal_id
that in production is derived from HNSW-configured vectors; update the fixture
to reflect the production path by switching the vector index to an HNSW index
(replace Indexes::Plain with the appropriate Indexes::Hnsw configuration in the
SegmentConfig/VectorDataConfig for DEFAULT_VECTOR_NAME and ensure any required
HNSW config fields are provided), or if you intentionally want an artificial
plain-only state, rename the helper and its docstring to explicitly state it’s a
synthetic test case that does not reflect HNSW-derived deferred IDs (update
function name build_segment_with_deferred_1 and its comment accordingly) so the
intent is clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8c25874-9f87-4097-88c9-9d3cdf6aaff4
📒 Files selected for processing (24)
lib/collection/src/collection_manager/fixtures.rslib/collection/src/collection_manager/optimizers/config_mismatch_optimizer.rslib/collection/src/collection_manager/optimizers/indexing_optimizer.rslib/collection/src/collection_manager/optimizers/merge_optimizer.rslib/collection/src/collection_manager/optimizers/vacuum_optimizer.rslib/collection/src/config.rslib/collection/src/optimizers_builder.rslib/collection/src/shards/local_shard/mod.rslib/collection/src/shards/local_shard/snapshot.rslib/collection/src/tests/mod.rslib/collection/src/update_workers/optimization_worker.rslib/edge/src/optimize.rslib/segment/src/segment/entry.rslib/segment/src/segment/mod.rslib/segment/src/segment/segment_ops.rslib/segment/src/segment/tests.rslib/segment/src/segment_constructor/segment_builder.rslib/segment/src/segment_constructor/segment_constructor_base.rslib/shard/src/fixtures.rslib/shard/src/operations/optimization.rslib/shard/src/optimize.rslib/shard/src/optimizers/segment_optimizer.rslib/shard/src/segment_holder/mod.rslib/shard/src/segment_holder/snapshot.rs
timvisee
approved these changes
Mar 9, 2026
| } | ||
| } | ||
| if let Some(deferred_from) = self.deferred_internal_id { | ||
| return self.is_appendable() && self.total_point_count() > deferred_from as usize; |
Member
There was a problem hiding this comment.
Was a bit confused first and thought we needed >=. But we index from zero, so it is correct as is.
IvanPleshkov
force-pushed
the
use-predefined-deferred-internal-id
branch
from
e1c76ee to
13ba740
Compare
Contributor
📝 WalkthroughWalkthroughThis pull request renames a threshold field across the optimizer and segment systems from Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/edge/src/config/shard.rs (1)
203-212:⚠️ Potential issue | 🔴 CriticalExtract and reuse deferred-point calculation in edge shards.
EdgeOptimizersConfigexposesprevent_unoptimized, butoptimizer_thresholds()hardcodesdeferred_internal_id: None. Unlike the collection path, edge shards never compute a deferred point ID from theprevent_unoptimizedflag and indexing threshold, so deferred-point blocking is non-functional for edge HNSW segments.The collection-side code (lib/collection/src/optimizers_builder.rs) computes this via
get_deferred_points_threshold_bytes()→collection_params.get_deferred_point_id(). The edge path must do the same.Options:
- Extract
get_deferred_points_threshold_bytes()from lib/shard and reuse it in edge, then callcollection_params.get_deferred_point_id()inoptimizer_thresholds().- Add a helper to
EdgeOptimizersConfigmirroringget_deferred_points_threshold_bytes().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/edge/src/config/shard.rs` around lines 203 - 212, optimizer_thresholds currently hardcodes deferred_internal_id: None causing prevent_unoptimized on EdgeOptimizersConfig to be ignored; compute the deferred point ID like the collection path by reusing the deferred-threshold logic and calling collection_params.get_deferred_point_id() (or add a mirror helper on EdgeOptimizersConfig). Specifically, obtain the deferred bytes via get_deferred_points_threshold_bytes(...) (extract and reuse it from lib/shard or add equivalent to EdgeOptimizersConfig), pass that and the indexing_threshold_kb (converted to bytes) into collection_params.get_deferred_point_id(), and set OptimizerThresholds.deferred_internal_id to the returned Option so edge shards honor prevent_unoptimized in optimizer_thresholds.
🧹 Nitpick comments (2)
lib/segment/src/segment/tests.rs (1)
830-847: Add a non-appendable regression case here.This reopen block still only validates the appendable path. Since
load_segmentis givenSome(DEFERRED_POINTS_ID)again on Line 834, it does not exercise the newself.is_appendable()gate inpoint_is_deferred(). Please add a case that builds or loads a non-appendable segment and asserts those points are no longer treated as deferred there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/segment/src/segment/tests.rs` around lines 830 - 847, Add a regression test that exercises the non-appendable path: after creating the appendable segment test you must also create or load a non-appendable segment (so that segment.is_appendable() returns false) and reopen it without passing Some(DEFERRED_POINTS_ID) to load_segment; then assert that segment.has_deferred_points() is false and segment.deferred_internal_id is None (or not Some(DEFERRED_POINTS_ID)). Locate the relevant code near load_segment, point_is_deferred(), is_appendable(), DEFERRED_POINTS_ID, deferred_internal_id, and has_deferred_points() and add the new case that builds/loads a non-appendable segment and verifies deferred points are not treated as deferred there.lib/collection/src/collection_manager/optimizers/indexing_optimizer.rs (1)
137-142: Please add one non-Nonedeferred-ID case here.All of the updated threshold fixtures in this file still set
deferred_internal_id: None, so the new appendable-segment path can regress without a failing optimizer test. A smallSome(...)case intest_multi_vector_optimizationwould lock down the new plumbing where this PR matters most.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/collection/src/collection_manager/optimizers/indexing_optimizer.rs` around lines 137 - 142, Add a test case where OptimizerThresholds.deferred_internal_id is not None to exercise the appendable-segment path: modify the thresholds used in test_multi_vector_optimization to include a small Some(<valid id>) value instead of None (e.g., a simple internal id used elsewhere in tests), so the new appendable-segment plumbing is exercised; update any setup/fixtures that construct OptimizerThresholds to pass this Some(...) case while keeping other threshold values the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/segment/src/segment_constructor/segment_constructor_base.rs`:
- Around line 865-869: Update the rustdoc for the public function build_segment
to include the new deferred_internal_id parameter: describe its type
(Option<PointOffsetType>), meaning (what a deferred internal id represents), and
how it affects behavior when Some(...) vs None, alongside the existing
descriptions for segments_path, config, and ready so the function signature and
docs stay in sync with the public API.
---
Outside diff comments:
In `@lib/edge/src/config/shard.rs`:
- Around line 203-212: optimizer_thresholds currently hardcodes
deferred_internal_id: None causing prevent_unoptimized on EdgeOptimizersConfig
to be ignored; compute the deferred point ID like the collection path by reusing
the deferred-threshold logic and calling
collection_params.get_deferred_point_id() (or add a mirror helper on
EdgeOptimizersConfig). Specifically, obtain the deferred bytes via
get_deferred_points_threshold_bytes(...) (extract and reuse it from lib/shard or
add equivalent to EdgeOptimizersConfig), pass that and the indexing_threshold_kb
(converted to bytes) into collection_params.get_deferred_point_id(), and set
OptimizerThresholds.deferred_internal_id to the returned Option so edge shards
honor prevent_unoptimized in optimizer_thresholds.
---
Nitpick comments:
In `@lib/collection/src/collection_manager/optimizers/indexing_optimizer.rs`:
- Around line 137-142: Add a test case where
OptimizerThresholds.deferred_internal_id is not None to exercise the
appendable-segment path: modify the thresholds used in
test_multi_vector_optimization to include a small Some(<valid id>) value instead
of None (e.g., a simple internal id used elsewhere in tests), so the new
appendable-segment plumbing is exercised; update any setup/fixtures that
construct OptimizerThresholds to pass this Some(...) case while keeping other
threshold values the same.
In `@lib/segment/src/segment/tests.rs`:
- Around line 830-847: Add a regression test that exercises the non-appendable
path: after creating the appendable segment test you must also create or load a
non-appendable segment (so that segment.is_appendable() returns false) and
reopen it without passing Some(DEFERRED_POINTS_ID) to load_segment; then assert
that segment.has_deferred_points() is false and segment.deferred_internal_id is
None (or not Some(DEFERRED_POINTS_ID)). Locate the relevant code near
load_segment, point_is_deferred(), is_appendable(), DEFERRED_POINTS_ID,
deferred_internal_id, and has_deferred_points() and add the new case that
builds/loads a non-appendable segment and verifies deferred points are not
treated as deferred there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53758852-03fb-4c15-ac37-19490420b6b2
📒 Files selected for processing (24)
lib/collection/src/collection_manager/fixtures.rslib/collection/src/collection_manager/optimizers/config_mismatch_optimizer.rslib/collection/src/collection_manager/optimizers/indexing_optimizer.rslib/collection/src/collection_manager/optimizers/merge_optimizer.rslib/collection/src/collection_manager/optimizers/vacuum_optimizer.rslib/collection/src/config.rslib/collection/src/optimizers_builder.rslib/collection/src/shards/local_shard/mod.rslib/collection/src/shards/local_shard/snapshot.rslib/collection/src/tests/mod.rslib/collection/src/update_workers/optimization_worker.rslib/edge/src/config/shard.rslib/segment/src/segment/entry.rslib/segment/src/segment/mod.rslib/segment/src/segment/segment_ops.rslib/segment/src/segment/tests.rslib/segment/src/segment_constructor/segment_builder.rslib/segment/src/segment_constructor/segment_constructor_base.rslib/shard/src/fixtures.rslib/shard/src/operations/optimization.rslib/shard/src/optimize.rslib/shard/src/optimizers/segment_optimizer.rslib/shard/src/segment_holder/mod.rslib/shard/src/segment_holder/snapshot.rs
Comment on lines
865
to
869
| pub fn build_segment( | ||
| segments_path: &Path, | ||
| config: &SegmentConfig, | ||
| deferred_points_threshold_bytes: Option<NonZeroUsize>, | ||
| deferred_internal_id: Option<PointOffsetType>, | ||
| ready: bool, |
Contributor
There was a problem hiding this comment.
Document the new deferred_internal_id parameter in build_segment.
The rustdoc argument list above this public function still mentions only segments_path, config, and ready, so the signature change is now undocumented for callers.
📝 Proposed doc update
/// # Arguments
///
/// * `segments_path` - Path to the segments directory. Segment folder will be created in this directory
/// * `config` - Segment configuration
+/// * `deferred_internal_id` - Deferred cutoff for appendable segments; ignored for non-appendable segments
/// * `ready` - Whether the segment is ready after building; will save segment version
///
/// To load a segment, saving the segment version is required. If `ready` is false, the version🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/segment/src/segment_constructor/segment_constructor_base.rs` around lines
865 - 869, Update the rustdoc for the public function build_segment to include
the new deferred_internal_id parameter: describe its type
(Option<PointOffsetType>), meaning (what a deferred internal id represents), and
how it affects behavior when Some(...) vs None, alongside the existing
descriptions for segments_path, config, and ready so the function signature and
docs stay in sync with the public API.
This was referenced Mar 10, 2026
This was referenced Mar 17, 2026
Merged
generall
pushed a commit
that referenced
this pull request
Mar 26, 2026
* Adjust points selection for deferred points update * adjust proxy segment implementation * simplify * use simpler proxy impl * stick to Entry API * renaming to stay closer to the original * two passes and simpler impl. * fmt * fmt * use predefined deferred internal id * calculate deferred point id * move deferred check to the entry * fix after rebase * fmt * fix tests * review remarks * fix tests * codespell fix * are you happy clippy --------- Co-authored-by: Arnaud Gourlay <arnaud.gourlay@gmail.com>
KShivendu
pushed a commit
that referenced
this pull request
Mar 30, 2026
* Adjust points selection for deferred points update * adjust proxy segment implementation * simplify * use simpler proxy impl * stick to Entry API * renaming to stay closer to the original * two passes and simpler impl. * fmt * fmt * use predefined deferred internal id * calculate deferred point id * move deferred check to the entry * fix after rebase * fmt * fix tests * review remarks * fix tests * codespell fix * are you happy clippy --------- Co-authored-by: Arnaud Gourlay <arnaud.gourlay@gmail.com>
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.
In dev we calculate the deferred id dynamically. It was a requirement for multivectors where the size of the vector is unknown.
But a dynamic approach doesn't solve the problem with deferred multivectors. Moreover, we know the deferred status of the point only after insertion.
After brainstorm it was decided to use 16 inner vectors as a constant for deferred id calculation. Such a decision makes everything easier and allows the use of deferred id for HNSW-indexed vectors only.
In this PR we calculate the deferred point id on the collection-config level and provide it to the segment, where we apply it only for appendable segments.
PR Pros:
PR Cons: