Skip to content

[opt](Ann) Cancel index building if input rows is less than the min_train_rows #60358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yiguolei merged 12 commits into from
Mar 25, 2026
Merged

[opt](Ann) Cancel index building if input rows is less than the min_train_rows #60358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
499284a
Dev
RoanHeNaN Jan 29, 2026
e3c93b0
fix range search
RoanHeNaN Jan 29, 2026
9244d95
fmt
RoanHeNaN Jan 30, 2026
ed3072f
Fix ut
RoanHeNaN Jan 30, 2026
b609cef
fix case
RoanHeNaN Jan 30, 2026
817b9b7
remove index from compound file
RoanHeNaN Jan 31, 2026
b27cd09
Fix code review issues for PR #60358
RoanHeNaN Mar 3, 2026
afebd66
opt mformat
RoanHeNaN Mar 3, 2026
c5f1c83
refine
RoanHeNaN Mar 11, 2026
895aae1
fix compile
RoanHeNaN Mar 17, 2026
d6cd966
fix review
RoanHeNaN Mar 19, 2026
7d4eebc
de
RoanHeNaN Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions be/src/exec/operator/olap_scan_operator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,8 @@ Status OlapScanLocalState::_init_profile() {
_ann_range_result_convert_costs =
ADD_CHILD_TIMER(_segment_profile, "AnnIndexRangeResultConvertCosts",
"AnnIndexRangeResultPostProcessCosts");
_ann_fallback_brute_force_cnt =
ADD_COUNTER(_segment_profile, "AnnIndexFallbackBruteForceCnt", TUnit::UNIT);
_variant_scan_sparse_column_timer = ADD_TIMER(_segment_profile, "VariantScanSparseColumnTimer");
_variant_scan_sparse_column_bytes =
ADD_COUNTER(_segment_profile, "VariantScanSparseColumnBytes", TUnit::BYTES);
Expand Down
2 changes: 2 additions & 0 deletions be/src/exec/operator/olap_scan_operator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,8 @@ class OlapScanLocalState final : public ScanLocalState<OlapScanLocalState> {
RuntimeProfile::Counter* _ann_range_engine_convert_costs = nullptr;
RuntimeProfile::Counter* _ann_range_result_convert_costs = nullptr;

RuntimeProfile::Counter* _ann_fallback_brute_force_cnt = nullptr;

RuntimeProfile::Counter* _output_index_result_column_timer = nullptr;

// number of segment filtered by column stat when creating seg iterator
Expand Down
2 changes: 2 additions & 0 deletions be/src/exec/scan/olap_scanner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -946,6 +946,8 @@ void OlapScanner::_collect_profile_before_close() {
COUNTER_UPDATE(local_state->_ann_topn_result_convert_costs,
stats.ann_index_topn_result_process_ns);

COUNTER_UPDATE(local_state->_ann_fallback_brute_force_cnt, stats.ann_fall_back_brute_force_cnt);

// Overhead counter removed; precise instrumentation is reported via engine_prepare above.
}

Expand Down
16 changes: 15 additions & 1 deletion be/src/exprs/vectorized_fn_call.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,13 +610,27 @@ Status VectorizedFnCall::evaluate_ann_range_search(
range_search_runtime.dim, index_dim);
}

auto stats = std::make_unique<segment_v2::AnnIndexStats>();
// Track load index timing
{
SCOPED_TIMER(&(stats->load_index_costs_ns));
if (!ann_index_iterator->try_load_index()) {
VLOG_DEBUG << "ANN range search skipped: "
<< fmt::format("Failed to load ANN index for column cid {}", src_col_cid);
ann_index_stats.fall_back_brute_force_cnt += 1;
return Status::OK();
}
double load_costs_ms = static_cast<double>(stats->load_index_costs_ns.value()) / 1000000.0;

@airborne12 airborne12 Mar 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

P2 — Range search fallback missing ann_fall_back_brute_force_cnt increment

When try_load_index() returns false here, the code returns Status::OK() without incrementing any fallback counter. The TopN path in segment_iterator.cpp consistently increments ann_fall_back_brute_force_cnt at every fallback branch (7 places). This observability gap makes it harder to diagnose range-search fallback scenarios in query profiles.

Consider adding a stats counter increment here as well.

DorisMetrics::instance()->ann_index_load_costs_ms->increment(
static_cast<int64_t>(load_costs_ms));
}

AnnRangeSearchParams params = range_search_runtime.to_range_search_params();

params.roaring = &row_bitmap;
DCHECK(params.roaring != nullptr);
DCHECK(params.query_value != nullptr);
segment_v2::AnnRangeSearchResult result;
auto stats = std::make_unique<segment_v2::AnnIndexStats>();
RETURN_IF_ERROR(ann_index_iterator->range_search(params, range_search_runtime.user_params,
&result, stats.get()));

Expand Down
11 changes: 11 additions & 0 deletions be/src/storage/index/ann/ann_index.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,17 @@ class VectorIndex {
*/
virtual doris::Status add(Int64 n, const float* x) = 0;

/**
* @brief Returns the minimum number of rows required for training the index.
*
* Some index types (like IVF) require a minimum number of training points.
* For example, IVF requires at least 'nlist' training points.
* HNSW does not require any minimum and returns 0.
*
* @return Minimum number of rows required for training
*/
virtual Int64 get_min_train_rows() const { return 0; }

/** Return approximate nearest neighbors of a query vector.
* The result is stored in the result object.
* @param query_vec input vector, size d
Expand Down
11 changes: 11 additions & 0 deletions be/src/storage/index/ann/ann_index_iterator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,17 @@ AnnIndexIterator::AnnIndexIterator(const IndexReaderPtr& reader) : IndexIterator
_ann_reader = std::dynamic_pointer_cast<AnnIndexReader>(reader);
}

bool AnnIndexIterator::try_load_index() {
if (_ann_reader == nullptr) {
LOG(WARNING) << "AnnIndexIterator::try_load_index: _ann_reader is null";
return false;
}

// _context may be unset in some test scenarios; pass nullptr IOContext in that case.
io::IOContext* io_ctx = (_context != nullptr) ? _context->io_ctx : nullptr;
return _ann_reader->try_load_index(io_ctx);
}

Status AnnIndexIterator::read_from_index(const IndexParam& param) {
auto* a_param = std::get<segment_v2::AnnTopNParam*>(param);
if (a_param == nullptr) {
Expand Down
4 changes: 4 additions & 0 deletions be/src/storage/index/ann/ann_index_iterator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ class AnnIndexIterator : public IndexIterator {

Result<bool> has_null() override { return true; }

// Try to load index, return true if successful, false if failed
// This method should be called before read_from_index or range_search
bool try_load_index();

MOCK_FUNCTION Status range_search(const AnnRangeSearchParams& params,
const VectorSearchUserParams& custom_params,
AnnRangeSearchResult* result, AnnIndexStats* stats);
Expand Down
34 changes: 18 additions & 16 deletions be/src/storage/index/ann/ann_index_reader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ Status AnnIndexReader::load_index(io::IOContext* io_ctx) {
DorisMetrics::instance()->ann_index_load_cnt->increment(1);

try {
// An exception will be thrown if loading fails
RETURN_IF_ERROR(
_index_file_reader->init(config::inverted_index_read_buffer_size, io_ctx));
Result<std::unique_ptr<DorisCompoundReader, DirectoryDeleter>> compound_dir;
Expand All @@ -87,23 +88,30 @@ Status AnnIndexReader::load_index(io::IOContext* io_ctx) {
_vector_index->set_type(_index_type);
RETURN_IF_ERROR(_vector_index->load(compound_dir->get()));
} catch (CLuceneError& err) {
LOG_ERROR("Failed to load ann index: {}", err.what());
return Status::Error<ErrorCode::INVERTED_INDEX_CLUCENE_ERROR>(
"CLuceneError occur when open ann idx file, error msg: {}", err.what());
}
return Status::OK();
});
}

Status AnnIndexReader::query(io::IOContext* io_ctx, AnnTopNParam* param, AnnIndexStats* stats) {
bool AnnIndexReader::try_load_index(io::IOContext* io_ctx) {
#ifndef BE_TEST
{
SCOPED_TIMER(&(stats->load_index_costs_ns));
RETURN_IF_ERROR(load_index(io_ctx));
double load_costs_ms = static_cast<double>(stats->load_index_costs_ns.value()) / 1000.0;
DorisMetrics::instance()->ann_index_load_costs_ms->increment(
static_cast<int64_t>(load_costs_ms));
Status st = load_index(io_ctx);

@github-actions github-actions Bot Mar 3, 2026

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.

Lost metrics tracking. The original code had:

SCOPED_TIMER(&(stats->load_index_costs_ns));
RETURN_IF_ERROR(load_index(io_ctx));
double load_costs_ms = ...
DorisMetrics::instance()->ann_index_load_costs_ms->increment(...);

By moving load to try_load_index() (which has no stats parameter), both load_index_costs_ns timing and ann_index_load_costs_ms metrics are completely lost. You should either pass the stats object to try_load_index() or record the timing at the call site in segment_iterator.cpp.

@airborne12 airborne12 Mar 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@zhiqiang-hhhh need check this

@RoanHeNaN RoanHeNaN Mar 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@zhiqiang-hhhh need check this

fixed https://github.com/apache/doris/pull/60358/changes#diff-923256ad78af49c7668b648b83bf66fb96c9d32bbaa497a59516d1066e27310eR831

if (!st.ok()) {
LOG_WARNING("Failed to load ann index, will fallback to brute force search: {}",
st.to_string());
return false;
}
#endif
return true;
}

Status AnnIndexReader::query(io::IOContext* io_ctx, AnnTopNParam* param, AnnIndexStats* stats) {
// Index should be loaded before calling query
DCHECK(_vector_index != nullptr);

{
DorisMetrics::instance()->ann_index_search_cnt->increment(1);
SCOPED_TIMER(&(stats->search_costs_ns));
Expand Down Expand Up @@ -162,16 +170,10 @@ Status AnnIndexReader::range_search(const AnnRangeSearchParams& params,
const VectorSearchUserParams& custom_params,
segment_v2::AnnRangeSearchResult* result,
segment_v2::AnnIndexStats* stats, io::IOContext* io_ctx) {
// Index should be loaded before calling range_search
DCHECK(_vector_index != nullptr);

DCHECK(stats != nullptr);
#ifndef BE_TEST
{
SCOPED_TIMER(&(stats->load_index_costs_ns));
RETURN_IF_ERROR(load_index(io_ctx));
double load_costs_ms = static_cast<double>(stats->load_index_costs_ns.value()) / 1000.0;
DorisMetrics::instance()->ann_index_load_costs_ms->increment(
static_cast<int64_t>(load_costs_ms));
}
#endif
{
DorisMetrics::instance()->ann_index_search_cnt->increment(1);
SCOPED_TIMER(&(stats->search_costs_ns));
Expand Down
4 changes: 4 additions & 0 deletions be/src/storage/index/ann/ann_index_reader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ class AnnIndexReader : public IndexReader {

Status load_index(io::IOContext* io_ctx);

// Try to load index, return true if successful, false if failed
// This method is used to check if index can be loaded before query
bool try_load_index(io::IOContext* io_ctx);

Status query(io::IOContext* io_ctx, AnnTopNParam* param, AnnIndexStats* stats);

Status range_search(const AnnRangeSearchParams& params,
Expand Down
52 changes: 46 additions & 6 deletions be/src/storage/index/ann/ann_index_writer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ Status AnnIndexColumnWriter::add_array_values(size_t field_size, const void* val
RETURN_IF_ERROR(
_vector_index->add(AnnIndexColumnWriter::chunk_size(), _float_array.data()));
_float_array.clear();
_need_save_index = true;
}
}

Expand All @@ -151,16 +152,55 @@ int64_t AnnIndexColumnWriter::size() const {
}

Status AnnIndexColumnWriter::finish() {
Int64 min_train_rows = _vector_index->get_min_train_rows();

// Check if we have enough rows to train the index
// train/add the remaining data
if (!_float_array.empty()) {
if (_float_array.empty()) {
if (_need_save_index) {
return _vector_index->save(_dir.get());
} else {
// No data was added at all. This can happen if the segment has 0 rows
// or all rows were filtered out. We need to delete the directory entry
// to avoid writing an empty/invalid index file.
LOG_INFO("No data to train/add for ANN index. Skipping index building.");
return _index_file_writer->delete_index(_index_meta);
}
} else {
DCHECK(_float_array.size() % _vector_index->get_dimension() == 0);

Int64 num_rows = _float_array.size() / _vector_index->get_dimension();
RETURN_IF_ERROR(_vector_index->train(num_rows, _float_array.data()));
RETURN_IF_ERROR(_vector_index->add(num_rows, _float_array.data()));
_float_array.clear();
}

return _vector_index->save(_dir.get());
if (num_rows >= min_train_rows) {
RETURN_IF_ERROR(_vector_index->train(num_rows, _float_array.data()));
RETURN_IF_ERROR(_vector_index->add(num_rows, _float_array.data()));
_float_array.clear();
return _vector_index->save(_dir.get());
} else {
// It happens to have not enough data to train.
// If we have data to add before, we still need to save the index.
if (_need_save_index) {
// For IVF indexes, adding remaining vectors without training is acceptable
// because the quantizer was already trained on previous batches. These vectors
// are simply added to the nearest clusters without retraining.
RETURN_IF_ERROR(_vector_index->add(num_rows, _float_array.data()));

@github-actions github-actions Bot Mar 3, 2026

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.

Potential correctness concern: When _need_save_index == true (previous chunks were trained+added) but the remaining rows are fewer than min_train_rows, you call add() without train(). For IVF indexes, this means the remaining vectors are added to an index whose quantizer was trained on previous batches. This is generally fine for FAISS since the quantizer is already trained, but it means these vectors won't benefit from the last batch's clustering. Worth a comment explaining this is intentional.

@airborne12 airborne12 Mar 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@zhiqiang-hhhh need check this

@RoanHeNaN RoanHeNaN Mar 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@zhiqiang-hhhh need check this

A comment has been added.

_float_array.clear();
return _vector_index->save(_dir.get());
} else {
// Not enough data to train and no data added before.
// Means this is a very small segment, we can skip the index building.
// We need to delete the directory entry from index_file_writer to avoid
// writing an empty/invalid index file which causes "IndexInput read past EOF" error.
LOG_INFO(
"Remaining data size {} is less than minimum {} rows required for ANN "
"index "
"training. Skipping index building for this segment.",
num_rows, min_train_rows);
_float_array.clear();
return _index_file_writer->delete_index(_index_meta);
}
}
}
}
#include "common/compile_check_end.h"
} // namespace doris::segment_v2
3 changes: 3 additions & 0 deletions be/src/storage/index/ann/ann_index_writer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,13 @@ class AnnIndexColumnWriter : public IndexColumnWriter {
// VectorIndex should be weak shared by AnnIndexWriter and VectorIndexReader
// This should be a weak_ptr
std::shared_ptr<VectorIndex> _vector_index;
// _float_array is used to buffer the float data before training/adding to vector index
// if we dont do this, the performance(recall) will be very poor when adding small number of vectors one by one
PODArray<float> _float_array;
IndexFileWriter* _index_file_writer;
const TabletIndex* _index_meta;
std::shared_ptr<DorisFSDirectory> _dir;
bool _need_save_index = false;
};
#include "common/compile_check_end.h"
} // namespace doris::segment_v2
10 changes: 7 additions & 3 deletions be/src/storage/index/ann/ann_search_params.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,17 @@ struct AnnIndexStats {
engine_search_ns(TUnit::TIME_NS, 0),
result_process_costs_ns(TUnit::TIME_NS, 0),
engine_convert_ns(TUnit::TIME_NS, 0),
engine_prepare_ns(TUnit::TIME_NS, 0) {}
engine_prepare_ns(TUnit::TIME_NS, 0),
fall_back_brute_force_cnt(0) {}

AnnIndexStats(const AnnIndexStats& other)
: search_costs_ns(TUnit::TIME_NS, other.search_costs_ns.value()),
load_index_costs_ns(TUnit::TIME_NS, other.load_index_costs_ns.value()),
engine_search_ns(TUnit::TIME_NS, other.engine_search_ns.value()),
result_process_costs_ns(TUnit::TIME_NS, other.result_process_costs_ns.value()),
engine_convert_ns(TUnit::TIME_NS, other.engine_convert_ns.value()),
engine_prepare_ns(TUnit::TIME_NS, other.engine_prepare_ns.value()) {}
engine_prepare_ns(TUnit::TIME_NS, other.engine_prepare_ns.value()),
fall_back_brute_force_cnt(other.fall_back_brute_force_cnt) {}

AnnIndexStats& operator=(const AnnIndexStats& other) {
if (this != &other) {
Expand All @@ -67,6 +69,7 @@ struct AnnIndexStats {
result_process_costs_ns.set(other.result_process_costs_ns.value());
engine_convert_ns.set(other.engine_convert_ns.value());
engine_prepare_ns.set(other.engine_prepare_ns.value());
fall_back_brute_force_cnt = other.fall_back_brute_force_cnt;
}
return *this;
}
Expand All @@ -77,7 +80,8 @@ struct AnnIndexStats {
RuntimeProfile::Counter result_process_costs_ns; // time cost of processing search results
RuntimeProfile::Counter engine_convert_ns; // time cost of engine-side conversions
RuntimeProfile::Counter
engine_prepare_ns; // time cost before engine search (allocations, setup)
engine_prepare_ns; // time cost before engine search (allocations, setup)
int64_t fall_back_brute_force_cnt; // fallback count when ANN range search is bypassed
};

struct AnnTopNParam {
Expand Down
7 changes: 2 additions & 5 deletions be/src/storage/index/ann/ann_topn_runtime.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,15 +191,12 @@ Status AnnTopNRuntime::prepare(RuntimeState* state, const RowDescriptor& row_des
return Status::OK();
}

Status AnnTopNRuntime::evaluate_vector_ann_search(segment_v2::IndexIterator* ann_index_iterator,
Status AnnTopNRuntime::evaluate_vector_ann_search(segment_v2::AnnIndexIterator* ann_index_iterator,
roaring::Roaring* roaring, size_t rows_of_segment,
IColumn::MutablePtr& result_column,
std::unique_ptr<std::vector<uint64_t>>& row_ids,
segment_v2::AnnIndexStats& ann_index_stats) {
DCHECK(ann_index_iterator != nullptr);
segment_v2::AnnIndexIterator* ann_index_iterator_casted =
dynamic_cast<segment_v2::AnnIndexIterator*>(ann_index_iterator);
DCHECK(ann_index_iterator_casted != nullptr);
DCHECK(_order_by_expr_ctx != nullptr);
DCHECK(_order_by_expr_ctx->root() != nullptr);
size_t query_array_size = _query_array->size();
Expand All @@ -209,7 +206,7 @@ Status AnnTopNRuntime::evaluate_vector_ann_search(segment_v2::IndexIterator* ann

// TODO:(zhiqiang) Maybe we can move this dimension check to prepare phase.

auto index_reader = ann_index_iterator_casted->get_reader(AnnIndexReaderType::ANN);
auto index_reader = ann_index_iterator->get_reader(AnnIndexReaderType::ANN);
auto ann_index_reader = std::dynamic_pointer_cast<AnnIndexReader>(index_reader);
DCHECK(ann_index_reader != nullptr);
if (ann_index_reader->get_dimension() != query_array_size) {
Expand Down
7 changes: 4 additions & 3 deletions be/src/storage/index/ann/ann_topn_runtime.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
namespace doris::segment_v2 {
#include "common/compile_check_begin.h"
struct AnnIndexStats;
class AnnIndexIterator;

Result<IColumn::Ptr> extract_query_vector(std::shared_ptr<VExpr> arg_expr);

Expand All @@ -67,7 +68,7 @@ Result<IColumn::Ptr> extract_query_vector(std::shared_ptr<VExpr> arg_expr);
* - Thread-safe execution in parallel query contexts
*
* Typical usage in SQL:
* SELECT * FROM table ORDER BY l2_distance(vec_column, [1,2,3]) LIMIT 10;
* SELECT * FROM table ORDER BY l2_distance_approximate(vec_column, [1,2,3]) LIMIT 10;
*/
class AnnTopNRuntime {
ENABLE_FACTORY_CREATOR(AnnTopNRuntime);
Expand Down Expand Up @@ -116,7 +117,7 @@ class AnnTopNRuntime {
* @param ann_index_stats Statistics collector for performance monitoring
* @return Status indicating success or failure
*/
Status evaluate_vector_ann_search(segment_v2::IndexIterator* ann_index_iterator,
Status evaluate_vector_ann_search(segment_v2::AnnIndexIterator* ann_index_iterator,
roaring::Roaring* row_bitmap, size_t rows_of_segment,
IColumn::MutablePtr& result_column,
std::unique_ptr<std::vector<uint64_t>>& row_ids,
Expand Down Expand Up @@ -167,4 +168,4 @@ class AnnTopNRuntime {
doris::VectorSearchUserParams _user_params; ///< User-defined search parameters
};
#include "common/compile_check_end.h"
} // namespace doris::segment_v2
} // namespace doris::segment_v2
26 changes: 26 additions & 0 deletions be/src/storage/index/ann/faiss_ann_index.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,32 @@ doris::Status FaissVectorIndex::add(Int64 n, const float* vec) {
return doris::Status::OK();
}

Int64 FaissVectorIndex::get_min_train_rows() const {
// For IVF indexes, the minimum number of training points should be at least
// equal to the number of clusters (nlist). FAISS requires this for k-means clustering.
Int64 ivf_min = 0;
if (_params.index_type == FaissBuildParameter::IndexType::IVF) {
ivf_min = _params.ivf_nlist;
}

// Calculate minimum training rows required by the quantizer
Int64 quantizer_min = 0;
if (_params.quantizer == FaissBuildParameter::Quantizer::PQ) {
// For PQ, FAISS uses ksub = 2^pq_nbits and recommends ksub * 100 training vectors.
// This threshold depends on pq_nbits only (independent of pq_m).
// See code from contrib/faiss/faiss/impl/ProductQuantizer.cpp::65
quantizer_min = (1LL << _params.pq_nbits) * 100;

@airborne12 airborne12 Mar 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

P1 — PQ min_train_rows formula inconsistency in test comments

The actual formula here is (1LL << _params.pq_nbits) * 100, which does not involve pq_m. For example:

  • pq_nbits=8 → 256 * 100 = 25,600
  • pq_nbits=2 → 4 * 100 = 400

However, multiple test comments reference completely different formulas:

  • ann_index_writer_test.cpp:931: "requires 256 * 2^8 * 2 = 131072"
  • quantizer_min_train_rows.groovy:108: "requires pq_m * (1 << pq_nbits) * 256 = 2048"

Unit tests use mocked get_min_train_rows() so the mismatch is hidden. Regression tests insert data far exceeding the real threshold, passing trivially. If the formula changes incorrectly in the future, these tests won't catch it.

Suggestion: Fix all comments to match this formula, and use boundary-closer data sizes in regression tests.

} else if (_params.quantizer == FaissBuildParameter::Quantizer::SQ4 ||
_params.quantizer == FaissBuildParameter::Quantizer::SQ8) {
// For SQ, minimal training requirement as scalar quantization is simpler
quantizer_min = 1;

@github-actions github-actions Bot Mar 3, 2026

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.

Comment/code mismatch. Comment says "use a minimum of 20 training vectors, similar to IVF's nlist * 2 with nlist=10" but the code sets quantizer_min = 1. Should this be quantizer_min = 20? Or if 1 is correct, the comment should be updated.

}
// For FLAT, no minimum training data required

// Return the maximum of IVF and quantizer requirements
return std::max(ivf_min, quantizer_min);
}

void FaissVectorIndex::build(const FaissBuildParameter& params) {
_params = params;
_dimension = params.dim;
Expand Down
Loading
Loading