fix: block stale index entries after partial-schema merge_insert#6514
fix: block stale index entries after partial-schema merge_insert#6514westonpace wants to merge 1 commit intolance-format:mainfrom
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
This feature has come up once or twice before. Is there a reason this couldn't be implemented at the object_store level? For example, I think you could enable something like https://github.com/foyer-rs/foyer with an Or is this cache doing something more than pure data caching of the object_store layer? |
df35c75 to
663bdbd
Compare
| @@ -132,6 +142,11 @@ impl DeepSizeOf for IndexMetadata { | |||
| .map(|fragment_bitmap| fragment_bitmap.serialized_size()) | |||
| .unwrap_or(0) | |||
| + self.files.deep_size_of_children(context) | |||
There was a problem hiding this comment.
is line 144 sort of a no-op at this point? The last line is all that will be returned right and it doesn't make use of line 144?
There was a problem hiding this comment.
These lines are all added together (there is a + at the start of line 145)
663bdbd to
92b17e9
Compare
justinrmiller
left a comment
There was a problem hiding this comment.
Just a couple of comments to consider.
| created_at: curr_index_meta.created_at, | ||
| base_id: None, | ||
| files: curr_index_meta.files.clone(), | ||
| invalidated_fragment_bitmap: None, |
There was a problem hiding this comment.
For Keep, should invalidated_fragment_bitmap be preserved?
There was a problem hiding this comment.
Good point. Actually, we can go ahead and keep the bitmap in both cases in compaction. I've updated the code. I went ahead and added a unit test as well. If an fragment becomes invalidated we remove it from the index's fragment bitmap and this will prevent the fragment from compacting with other fragments so we should be safe on compaction.
| /// Perform a partial-schema merge_insert (only id + value_a) targeting specific id ranges. | ||
| /// This causes touched fragments to drop from the index bitmap while btree data retains | ||
| /// stale entries. | ||
| async fn partial_merge_insert( |
There was a problem hiding this comment.
invalidated_fragment_bitmap grows unbounded until a full rebuild right? Could that ever become an issue?
There was a problem hiding this comment.
It will never be larger than the number of fragments in the dataset which is reasonably bounded. In general, an index with any invalidated fragments is going to perform slightly worse than one without because we have to do the masking. That masking will be slightly more expensive when there are more fragments.
until a full rebuild
It will be cleared on an index update (optimize indices), not just a full rebuild.
LuQQiu
left a comment
There was a problem hiding this comment.
Generally looks good, besides the key problems @justinrmiller pointed out
| && let Err(e) = bitmap.serialize_into(&mut invalidated_fragment_bitmap) | ||
| { | ||
| log::error!("Failed to serialize invalidated fragment bitmap: {}", e); | ||
| invalidated_fragment_bitmap.clear(); |
There was a problem hiding this comment.
Ah, yeah, this is dangerous. I guess it was copying the logic from above. Still seems we should error in both cases. We will just need to convert this into a TryFrom
There was a problem hiding this comment.
I have converted it to a TryFrom
d0e534e to
d3fd26b
Compare
A partial-schema merge_insert that modifies indexed columns drops the affected fragments from the index's fragment_bitmap but leaves stale entries in the index data (btree, vector, inverted). Subsequent queries that use the index find rows via the stale entries AND via the unindexed-fragment scan, producing duplicates or errors. Add an `invalidated_fragment_bitmap` field to IndexMetadata that tracks fragments removed from the bitmap. At query time, all rows from these fragments are blocked by the deletion mask / prefilter so stale index entries are ignored. The bitmap is cleared when the index is rebuilt. Fixes three error categories seen in production: - "Ambiguous merge inserts: multiple source rows match the same target row" - "fragment id N does not exist in the dataset" - "Attempt to merge two RecordBatch with different sizes: N != 0" Also fixes duplicate results in vector search and full text search after partial-schema merge_insert. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
d3fd26b to
498c428
Compare
|
Merge pending vote on #6529 |
Summary
merge_insertthat modifies indexed columns drops the affected fragments from the index'sfragment_bitmapbut leaves stale entries in the index data (btree, vector, inverted). Subsequent queries that use the index find rows via both the stale entries AND the unindexed-fragment scan, producing duplicates or errors.invalidated_fragment_bitmapfield toIndexMetadata(proto + Rust) that tracks fragments removed from the bitmap duringprune_updated_fields_from_indices. At query time, all rows from these fragments are blocked by the deletion mask / prefilter so stale index entries are ignored. The bitmap is cleared when the index is rebuilt.Ambiguous merge inserts: multiple source rows match the same target row(221 occurrences)fragment id N does not exist in the dataset(114 occurrences)Attempt to merge two RecordBatch with different sizes: N != 0(165 occurrences)Test plan
test_partial_merge_insert_stale_index_ambiguous— second partial merge_insert finds same rows via stale btree + unindexed scantest_partial_merge_insert_stale_index_fragment_not_exist— partial merge + update-all + partial merge hits deleted fragmenttest_partial_merge_insert_stale_index_batch_size_mismatch— partial merge + partial update + partial merge hits deleted rowstest_partial_merge_insert_stale_vector_index_duplicates— KNN search returns duplicate rows after partial mergetest_partial_merge_insert_stale_fts_index_duplicates— FTS search returns duplicate rows after partial mergeCloses #6283
🤖 Generated with Claude Code