Skip to content

Seperate quantizer training from index build - #235

Merged
syvb merged 2 commits into
mainfrom
sv/quantize-seperately
Jun 10, 2025
Merged

syvb merged 2 commits into
mainfrom
sv/quantize-seperately

Conversation

@syvb

@syvb syvb commented Jun 6, 2025

Copy link
Copy Markdown
Contributor

This splits out quantizer training from do_heap_scan. This doesn't change any of the behaviour of index-building, but is going to be useful in a future PR that adds support for parallel indexing.

This PR also makes it so the creation progress is set properly when building a plain (non-quantized) index.

@syvb
syvb requested a review from a team as a code owner June 6, 2025 16:38
@syvb
syvb requested review from Copilot and tjgreen42 June 6, 2025 16:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR separates the quantizer training phase from the main heap scan in the index build process and ensures the index-creation progress is updated correctly for plain storage.

  • Removed start_training/add_sample/finish_training methods from the Storage trait and its implementations.
  • Introduced a standalone train_quantizer function (with BqTrainState) to handle quantizer training before graph building.
  • Updated ambuild and do_heap_scan to accept and forward WriteStats, reflecting the split between training and building.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/access_method/storage.rs Dropped unused training methods from the Storage trait.
src/access_method/sbq/storage.rs Switched SBQ storage to load a pretrained quantizer; removed inline training methods.
src/access_method/plain/storage.rs Removed no-op training stubs to align with trait cleanup.
src/access_method/graph/mod.rs Deleted get_meta_page_mut accessor as it’s no longer used.
src/access_method/build.rs Added train_quantizer, restructured ambuild for two-phase building, and updated do_heap_scan.
Comments suppressed due to low confidence (3)

pgvectorscale/src/access_method/build.rs:32

  • [nitpick] The struct name BqTrainState is inconsistent with the Sbq prefix used elsewhere; consider renaming it to SbqTrainState for clarity and consistency.
struct BqTrainState<'a, 'b> {

pgvectorscale/src/access_method/build.rs:277

  • [nitpick] Consider adding a doc comment to train_quantizer to explain its purpose, parameters, return value, and when it should be used.
fn train_quantizer(

pgvectorscale/src/access_method/build.rs:277

  • It would be beneficial to add unit or integration tests covering train_quantizer to ensure the quantizer training logic and metadata storage behave as expected.
fn train_quantizer(

Comment thread pgvectorscale/src/access_method/build.rs Outdated
Comment thread pgvectorscale/src/access_method/sbq/storage.rs
@syvb
syvb force-pushed the sv/quantize-seperately branch from ec1f506 to eb8521f Compare June 6, 2025 16:52
@syvb
syvb force-pushed the sv/quantize-seperately branch from eb8521f to 2876d29 Compare June 6, 2025 16:53
tjgreen42
tjgreen42 previously approved these changes Jun 6, 2025

@tjgreen42 tjgreen42 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me, modulo minor comment about naming!

Comment thread pgvectorscale/src/access_method/build.rs Outdated
@syvb
syvb force-pushed the sv/quantize-seperately branch from cdbf81b to 41384f9 Compare June 10, 2025 16:55
@syvb
syvb requested a review from tjgreen42 June 10, 2025 16:56
@syvb
syvb merged commit 0481047 into main Jun 10, 2025
@syvb
syvb deleted the sv/quantize-seperately branch June 10, 2025 17:20
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.

3 participants