fix(index): check quantizer params before sampling training data - #9368
LuciferYang wants to merge 5 commits into
Conversation
load_or_build_quantizer sampled and residual-transformed the full training dataset before checking whether quantizer build params were set, wasting the entire pass when they were missing. The Some(q) match arm after the early return was also dead code (the early return at the top of the function already handles that case). Move the params check before any IO and remove the dead arm. Assisted-by: GLM-5.3
|
Added |
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The follow-up closes the earlier coverage gap: the new test distinguishes validation-before-sampling from the old order by making sampling fail on an invalid column. The upfront guard is a focused fix and preserves the preloaded-quantizer path.
Closes #9367
load_or_build_quantizerreturns early when a quantizer is already set, so everything below that runs withself.quantizer == None. The tailmatch &self.quantizercould therefore only take itsNonearm, and that arm held the only check onquantizer_params— a builder given neither params nor a quantizer sampled the training data and computed residuals before failing. The params are now bound once at the top withlet ... else, the tail collapses to theQ::buildcall, andsample_size_hintreads from that binding.The
None => 256 * 256sample-size default went with it. Its comment says "here it must be retrain", but retrain arrives withquantizer: Some(..)and returns at the top, so that arm was unreachable; the case it was written for ended in the params error either way.No behaviour changes for any reachable input. The params-missing path is only reachable by calling
IvfIndexBuilder::new(.., None, ..)explicitly — the one constructor that leavesquantizer_paramsempty,new_incremental, setsquantizerat the same time — and that path produced the same error before, just later. So this is dead-code removal and a check moved earlier, not a fix for something a user can hit.How was this patch tested?
No new test: there is no reachable input whose behaviour differs, so there is nothing new to pin.
cargo test -p lance --lib index::vectorcovers the paths that do build a quantizer, including the incremental one that returns at the early exit.