From 4de76fdbde50975a0fe1f0cd2bde094b40188733 Mon Sep 17 00:00:00 2001 From: egolearner Date: Mon, 13 Jul 2026 10:54:30 +0800 Subject: [PATCH] fix(fts): allow global doc id gaps during compaction (#588) --- .../column/fts_column/fts_rocksdb_reducer.cc | 10 +-- .../column/fts_column/fts_rocksdb_reducer.h | 4 +- src/db/index/column/fts_column/fts_types.h | 4 +- tests/db/collection_test.cc | 63 +++++++++++++++++++ .../fts_column/fts_rocksdb_reducer_test.cc | 25 ++++++-- 5 files changed, 93 insertions(+), 13 deletions(-) diff --git a/src/db/index/column/fts_column/fts_rocksdb_reducer.cc b/src/db/index/column/fts_column/fts_rocksdb_reducer.cc index 21f3567..f46a8ca 100644 --- a/src/db/index/column/fts_column/fts_rocksdb_reducer.cc +++ b/src/db/index/column/fts_column/fts_rocksdb_reducer.cc @@ -132,12 +132,14 @@ Result FtsRocksdbReducer::feed( return {}; } - // Require consecutive global doc_id ranges between non-empty segments so - // the shared delete_row_id_bitmap stays aligned with input scan order. + // Global doc_id gaps are valid after deleted documents have been removed + // during compaction. Only require ordered, non-overlapping ranges; the + // shared delete bitmap is aligned by feed-order scan position and doc_count. if (!segment_stats_.empty() && - segment_stats.min_doc_id != segment_stats_.back().max_doc_id + 1) { + segment_stats.min_doc_id <= segment_stats_.back().max_doc_id) { return tl::make_unexpected(Status::InternalError( - "FtsRocksdbReducer: segments not in consecutive doc_id order. field=", + "FtsRocksdbReducer: segments have overlapping or unordered doc_id " + "ranges. field=", field_name_)); } diff --git a/src/db/index/column/fts_column/fts_rocksdb_reducer.h b/src/db/index/column/fts_column/fts_rocksdb_reducer.h index 02a6b47..c2e0c9f 100644 --- a/src/db/index/column/fts_column/fts_rocksdb_reducer.h +++ b/src/db/index/column/fts_column/fts_rocksdb_reducer.h @@ -52,7 +52,9 @@ class FtsRocksdbReducer { Result cleanup(); /*! Feed a source segment to be merged. - * Segments must be fed in consecutive doc_id order. + * Segments must be fed in ascending, non-overlapping global doc_id order. + * Gaps between global doc_id ranges are allowed because deleted documents + * may already have been removed by an earlier compaction. * \param segment_stats Stats of the source segment (min/max doc_id) * \param src_ctx RocksdbContext owning the source CFs * \param src_postings_cf Source postings CF (must be BitPacked) diff --git a/src/db/index/column/fts_column/fts_types.h b/src/db/index/column/fts_column/fts_types.h index 7d96a14..46eee46 100644 --- a/src/db/index/column/fts_column/fts_types.h +++ b/src/db/index/column/fts_column/fts_types.h @@ -37,8 +37,8 @@ struct FtsQueryParams { }; /*! Per-segment statistics needed by the FTS reducer for doc_id remapping. - * - min_doc_id / max_doc_id: GLOBAL doc_id range used by the delete filter - * (filter.is_filtered() takes a global doc_id). + * - min_doc_id / max_doc_id: GLOBAL doc_id range used to order segments and + * reject overlapping inputs. Gaps are valid after compaction deletes. * - doc_count: number of FTS LOCAL doc_ids in the source segment; the posting * list domain is [0, doc_count). For fresh (non-merged) segments this * equals max_doc_id - min_doc_id + 1, and the local-to-global mapping is diff --git a/tests/db/collection_test.cc b/tests/db/collection_test.cc index a5bdc73..ddb2320 100644 --- a/tests/db/collection_test.cc +++ b/tests/db/collection_test.cc @@ -5899,6 +5899,69 @@ TEST_F(CollectionTest, Feature_NoVectorCollection_FtsLifecycle) { FileHelper::RemoveDirectory(col_path); } +TEST_F(CollectionTest, Feature_FtsOptimizeAcceptsGlobalDocIdGaps) { + FileHelper::RemoveDirectory(col_path); + + auto schema = std::make_shared("fts_optimize_gaps"); + schema->set_max_doc_count_per_segment(1000); + schema->add_field(std::make_shared( + "content", DataType::STRING, false, std::make_shared())); + + auto create_res = Collection::CreateAndOpen(col_path, *schema, + CollectionOptions{false, true}); + ASSERT_TRUE(create_res.has_value()) << create_res.error().message(); + auto col = std::move(create_res.value()); + + for (uint64_t batch = 0; batch < 3; ++batch) { + std::vector docs; + docs.reserve(1000); + for (uint64_t i = 0; i < 1000; ++i) { + uint64_t id = batch * 1000 + i; + Doc doc; + doc.set_pk("pk_" + std::to_string(id)); + doc.set("content", "hello boundary"); + docs.emplace_back(std::move(doc)); + } + ASSERT_TRUE(col->Insert(docs).has_value()); + } + + auto delete_ranges = [&](uint64_t offset, uint64_t count) { + std::vector pks; + pks.reserve(count * 3); + for (uint64_t base : {0, 1000, 2000}) { + for (uint64_t i = 0; i < count; ++i) { + pks.emplace_back("pk_" + std::to_string(base + offset + i)); + } + } + auto result = col->Delete(pks); + ASSERT_TRUE(result.has_value()) << result.error().message(); + }; + + // The first rebuild removes the head of each source segment, producing + // persisted global ranges [400, 999], [1400, 1999], [2400, 2999]. + delete_ranges(0, 400); + ASSERT_TRUE(col->Optimize().ok()); + ASSERT_EQ(col->Stats().value().doc_count, 1800u); + + // A second round raises the delete ratio above the rebuild threshold and + // merges segments whose global ranges contain legitimate delete gaps. + delete_ranges(400, 200); + ASSERT_TRUE(col->Optimize().ok()); + ASSERT_EQ(col->Stats().value().doc_count, 1200u); + + SearchQuery query; + query.target_.field_name_ = "content"; + query.topk_ = 10; + FtsClause fts; + fts.query_string_ = "hello"; + query.target_.clause_ = fts; + auto result = col->Query(query); + ASSERT_TRUE(result.has_value()) << result.error().message(); + ASSERT_EQ(result.value().size(), 10u); + + ASSERT_TRUE(col->Destroy().ok()); +} + TEST_F(CollectionTest, Feature_NoVectorCollection_FtsReopenWithoutOptimize) { FileHelper::RemoveDirectory(col_path); diff --git a/tests/db/index/column/fts_column/fts_rocksdb_reducer_test.cc b/tests/db/index/column/fts_column/fts_rocksdb_reducer_test.cc index d860ba2..469bc49 100644 --- a/tests/db/index/column/fts_column/fts_rocksdb_reducer_test.cc +++ b/tests/db/index/column/fts_column/fts_rocksdb_reducer_test.cc @@ -333,22 +333,35 @@ TEST_F(FtsRocksdbReducerTest, FeedFailsBeforeInit) { .has_value()); } -TEST_F(FtsRocksdbReducerTest, FeedFailsWithNonConsecutiveDocIds) { +TEST_F(FtsRocksdbReducerTest, FeedAcceptsGapBetweenGlobalDocIdRanges) { FtsRocksdbReducer reducer = MakeReducer(); FtsSegmentStats stats0 = MakeSegmentStats(0, 2); EXPECT_TRUE(reducer.feed(stats0, &src0_db_, src0_postings_, src0_positions_) .has_value()); - // Gap: src1 starts at 4 instead of 3 + // Deletes may leave a gap between persisted segments. The reducer remaps + // segment-local doc_ids by feed-order scan position, not by global doc_id. FtsSegmentStats stats1 = MakeSegmentStats(4, 6); + EXPECT_TRUE(reducer.feed(stats1, &src1_db_, src1_postings_, src1_positions_) + .has_value()); +} + +TEST_F(FtsRocksdbReducerTest, FeedFailsWithOverlappingGlobalDocIdRanges) { + FtsRocksdbReducer reducer = MakeReducer(); + + FtsSegmentStats stats0 = MakeSegmentStats(0, 2); + EXPECT_TRUE(reducer.feed(stats0, &src0_db_, src0_postings_, src0_positions_) + .has_value()); + + FtsSegmentStats stats1 = MakeSegmentStats(2, 4); EXPECT_FALSE(reducer.feed(stats1, &src1_db_, src1_postings_, src1_positions_) .has_value()); } TEST_F(FtsRocksdbReducerTest, FeedAcceptsEmptySegmentAsNoop) { // Empty segments (doc_count == 0) silently contribute nothing — the - // surrounding non-empty segments still get their contiguity validated + // surrounding non-empty segments still get their ordering validated // against each other, as if the empty one wasn't there. auto indexer0 = MakeSrc0Indexer(); InsertDocs(indexer0.get(), {{0, "hello world"}, {1, "foo"}, {2, "bar"}}); @@ -361,7 +374,7 @@ TEST_F(FtsRocksdbReducerTest, FeedAcceptsEmptySegmentAsNoop) { ASSERT_TRUE(reducer.feed(stats0, &src0_db_, src0_postings_, src0_positions_) .has_value()); - // Empty middle segment — accepted, doesn't break contiguity. + // Empty middle segment — accepted, doesn't affect ordering. FtsSegmentStats empty_stats; empty_stats.min_doc_id = 0; empty_stats.max_doc_id = 0; @@ -370,8 +383,8 @@ TEST_F(FtsRocksdbReducerTest, FeedAcceptsEmptySegmentAsNoop) { reducer.feed(empty_stats, &src1_db_, src1_postings_, src1_positions_) .has_value()); - // src1 must still start at stats0.max_doc_id + 1 = 3, not be shifted by - // the (skipped) empty segment. + // The next non-empty segment is checked against stats0, not against the + // skipped empty segment. FtsSegmentStats stats1 = MakeSegmentStats(3, 3); ASSERT_TRUE(reducer.feed(stats1, &src1_db_, src1_postings_, src1_positions_) .has_value());