Resolve facet point visibility through the id tracker

The facet index previously took a `deferred_internal_id` and re-derived
point visibility from a raw offset threshold (`range_cardinality(..cutoff)`
/ `take_while(id < cutoff)`), duplicating logic owned by
`PointMappingsRefEnum::filter_deferred_and_deleted` and silently ignoring
the deleted bitslice on some paths.

Remove `deferred_internal_id` (and the per-variant `for_each_count_per_value`
it threaded through) from the `FacetIndex` / `MapIndexRead` / `BoolIndexRead`
surfaces. Facet counts are now computed exactly by walking each value's
posting and filtering it through `filter_deferred_and_deleted`
(`count_visible_posting`), the single source of truth for point-offset
visibility — deferred and soft-deleted points are excluded uniformly for
every index variant. `visit_facet_iter` and the no-filter `facet_values`
branch use the same path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
generall
2026-06-19 18:07:58 +02:00
parent 1bab9c8e9f
commit 22c657f984
13 changed files with 74 additions and 334 deletions

View File

@@ -17,7 +17,7 @@ use super::facet_index::FacetIndex;
use super::{PayloadFieldIndex, PayloadFieldIndexRead, ValueIndexer};
use crate::common::flags::roaring_flags::RoaringFlags;
use crate::common::operation_error::{OperationError, OperationResult};
use crate::data_types::facets::{FacetHit, FacetValue, FacetValueRef};
use crate::data_types::facets::{FacetValue, FacetValueRef};
use crate::index::payload_config::IndexMutability;
use crate::index::query_optimization::optimized_filter::DynConditionChecker;
use crate::index::query_optimization::rescore_formula::value_retriever::VariableRetrieverFn;
@@ -228,19 +228,6 @@ impl FacetIndex for BoolIndex {
f(FacetValue::Bool(b), iter)
})
}
fn for_each_count_per_value(
&self,
deferred_internal_id: Option<PointOffsetType>,
mut f: impl FnMut(FacetHit<FacetValueRef<'_>>) -> OperationResult<()>,
) -> OperationResult<()> {
BoolIndexRead::for_each_count_per_value(self, deferred_internal_id, |value, count| {
f(FacetHit {
value: FacetValueRef::Bool(value),
count,
})
})
}
}
impl ValueIndexer for BoolIndex {

View File

@@ -7,7 +7,7 @@ use super::super::read_ops::{self, BoolIndexRead};
use super::ReadOnlyBoolIndex;
use crate::common::flags::read_only_roaring_flags::ReadOnlyRoaringFlags;
use crate::common::operation_error::OperationResult;
use crate::data_types::facets::{FacetHit, FacetValue, FacetValueRef};
use crate::data_types::facets::{FacetValue, FacetValueRef};
use crate::index::field_index::facet_index::FacetIndex;
use crate::index::field_index::{
CardinalityEstimation, PayloadBlockCondition, PayloadFieldIndexRead,
@@ -150,17 +150,4 @@ impl<S: UniversalRead> FacetIndex for ReadOnlyBoolIndex<S> {
f(FacetValue::Bool(b), iter)
})
}
fn for_each_count_per_value(
&self,
deferred_internal_id: Option<PointOffsetType>,
mut f: impl FnMut(FacetHit<FacetValueRef<'_>>) -> OperationResult<()>,
) -> OperationResult<()> {
BoolIndexRead::for_each_count_per_value(self, deferred_internal_id, |value, count| {
f(FacetHit {
value: FacetValueRef::Bool(value),
count,
})
})
}
}

View File

@@ -143,32 +143,6 @@ pub trait BoolIndexRead {
Ok(())
}
fn for_each_count_per_value<F>(
&self,
deferred_internal_id: Option<PointOffsetType>,
mut f: F,
) -> OperationResult<()>
where
F: FnMut(bool, usize) -> OperationResult<()>,
{
let (false_count, true_count) = match deferred_internal_id {
Some(deferred_internal_id) => {
let false_count =
self.falses_flags()
.get_bitmap()
.range_cardinality(..deferred_internal_id) as usize;
let true_count =
self.trues_flags()
.get_bitmap()
.range_cardinality(..deferred_internal_id) as usize;
(false_count, true_count)
}
None => (self.falses_count(), self.trues_count()),
};
f(false, false_count)?;
f(true, true_count)
}
fn ram_usage_bytes(&self) -> usize {
self.trues_flags().get_bitmap().serialized_size()
+ self.falses_flags().get_bitmap().serialized_size()

View File

@@ -1,13 +1,12 @@
use common::counter::hardware_counter::HardwareCounterCell;
use common::types::PointOffsetType;
use common::universal_io::{MmapFile, UniversalRead};
use itertools::Itertools;
use super::bool_index::{BoolIndex, ReadOnlyBoolIndex};
use super::map_index::MapIndex;
use super::map_index::read_only::ReadOnlyMapIndex;
use crate::common::operation_error::OperationResult;
use crate::data_types::facets::{FacetHit, FacetValue, FacetValueRef};
use crate::data_types::facets::{FacetValue, FacetValueRef};
use crate::types::{IntPayloadType, UuidIntType};
pub trait FacetIndex {
@@ -51,60 +50,6 @@ pub trait FacetIndex {
&mut dyn Iterator<Item = PointOffsetType>,
) -> OperationResult<()>,
) -> OperationResult<()>;
/// Call a closure on each value->count mapping.
fn for_each_count_per_value(
&self,
deferred_internal_id: Option<PointOffsetType>,
f: impl FnMut(FacetHit<FacetValueRef<'_>>) -> OperationResult<()>,
) -> OperationResult<()>;
/// Candidate-restricted analog of [`Self::for_each_count_per_value`]: counts
/// only the given `values` (via [`Self::for_values_map`]). `deferred_internal_id`
/// behaves identically — `Some(threshold)` counts only points `< threshold`.
fn for_counts_per_value(
&self,
values: impl Iterator<Item = FacetValue>,
deferred_internal_id: Option<PointOffsetType>,
hw_counter: &HardwareCounterCell,
mut f: impl FnMut(FacetHit<FacetValue>) -> OperationResult<()>,
) -> OperationResult<()> {
let max_id = deferred_internal_id.unwrap_or(PointOffsetType::MAX);
self.for_values_map(values, hw_counter, |value, ids| {
// Postings are sorted, so `take_while` matches `range_cardinality(..threshold)`.
let count = ids.dedup().take_while(|&id| id < max_id).count();
f(FacetHit { value, count })
})
}
/// Like [`for_each_value`] but skips values whose only points are deferred.
///
/// When `deferred_internal_id` is `None`, this is equivalent to
/// [`for_each_value`]. When `Some(threshold)`, a value is reported only if
/// it has at least one point with internal id `< threshold`.
fn for_each_visible_value(
&self,
hw_counter: &HardwareCounterCell,
deferred_internal_id: Option<PointOffsetType>,
mut f: impl FnMut(FacetValueRef<'_>) -> OperationResult<()>,
) -> OperationResult<()> {
match deferred_internal_id {
Some(deferred_internal_id) => {
self.for_each_value_map(hw_counter, |facet_value, id_iter| {
let has_visible_point = id_iter
.take_while(|&id| id < deferred_internal_id)
.next()
.is_some();
if has_visible_point {
f(facet_value)?;
}
Ok(())
})
}
None => self.for_each_value(f),
}
}
}
/// Borrowed view over any concrete index that can produce facet counts.
@@ -256,37 +201,4 @@ impl<'a, S: UniversalRead> FacetIndex for FacetIndexEnum<'a, S> {
}
}
}
fn for_each_count_per_value(
&self,
deferred_internal_id: Option<PointOffsetType>,
f: impl FnMut(FacetHit<FacetValueRef<'_>>) -> OperationResult<()>,
) -> OperationResult<()> {
match self {
FacetIndexEnum::Keyword(index) => {
FacetIndex::for_each_count_per_value(*index, deferred_internal_id, f)
}
FacetIndexEnum::Int(index) => {
FacetIndex::for_each_count_per_value(*index, deferred_internal_id, f)
}
FacetIndexEnum::Uuid(index) => {
FacetIndex::for_each_count_per_value(*index, deferred_internal_id, f)
}
FacetIndexEnum::Bool(index) => {
FacetIndex::for_each_count_per_value(*index, deferred_internal_id, f)
}
FacetIndexEnum::KeywordReadOnly(index) => {
FacetIndex::for_each_count_per_value(*index, deferred_internal_id, f)
}
FacetIndexEnum::IntReadOnly(index) => {
FacetIndex::for_each_count_per_value(*index, deferred_internal_id, f)
}
FacetIndexEnum::UuidReadOnly(index) => {
FacetIndex::for_each_count_per_value(*index, deferred_internal_id, f)
}
FacetIndexEnum::BoolReadOnly(index) => {
FacetIndex::for_each_count_per_value(*index, deferred_internal_id, f)
}
}
}
}

View File

@@ -10,7 +10,7 @@ use super::key::MapIndexKey;
use super::read_only::ReadOnlyMapIndex;
use super::read_ops::MapIndexRead;
use crate::common::operation_error::OperationResult;
use crate::data_types::facets::{FacetHit, FacetValue, FacetValueRef};
use crate::data_types::facets::{FacetValue, FacetValueRef};
use crate::index::field_index::facet_index::FacetIndex;
impl<N: MapIndexKey + ?Sized> FacetIndex for MapIndex<N>
@@ -74,19 +74,6 @@ where
) -> OperationResult<()> {
MapIndexRead::for_values_map(self, values, hw_counter, f)
}
fn for_each_count_per_value(
&self,
deferred_internal_id: Option<PointOffsetType>,
mut f: impl FnMut(FacetHit<FacetValueRef<'_>>) -> OperationResult<()>,
) -> OperationResult<()> {
MapIndexRead::for_each_count_per_value(self, deferred_internal_id, |value, count| {
f(FacetHit {
value: value.into(),
count,
})
})
}
}
/// Faceting over the read-only enum mirrors [`FacetIndex for MapIndex<N>`]:
@@ -157,17 +144,4 @@ where
) -> OperationResult<()> {
MapIndexRead::for_values_map(self, values, hw_counter, f)
}
fn for_each_count_per_value(
&self,
deferred_internal_id: Option<PointOffsetType>,
mut f: impl FnMut(FacetHit<FacetValueRef<'_>>) -> OperationResult<()>,
) -> OperationResult<()> {
MapIndexRead::for_each_count_per_value(self, deferred_internal_id, |value, count| {
f(FacetHit {
value: value.into(),
count,
})
})
}
}

View File

@@ -75,20 +75,6 @@ where
self.value_to_points.keys().try_for_each(|v| f(v.borrow()))
}
fn for_each_count_per_value(
&self,
deferred_internal_id: Option<PointOffsetType>,
mut f: impl FnMut(&N, usize) -> OperationResult<()>,
) -> OperationResult<()> {
// Immutable indexes don't support deferred filtering; callers must
// pass `None`. See `MapIndex::for_each_count_per_value` for context.
debug_assert!(deferred_internal_id.is_none());
let _ = deferred_internal_id;
self.value_to_points
.iter()
.try_for_each(|(k, entry)| f(k.borrow(), entry.count as usize))
}
fn for_each_value_map(
&self,
_hw_counter: &HardwareCounterCell,

View File

@@ -168,20 +168,6 @@ where
self.map.keys().try_for_each(|v| f(v.borrow()))
}
fn for_each_count_per_value(
&self,
deferred_internal_id: Option<PointOffsetType>,
mut f: impl FnMut(&N, usize) -> OperationResult<()>,
) -> OperationResult<()> {
self.map.iter().try_for_each(|(k, v)| {
let count = match deferred_internal_id {
Some(deferred_internal_id) => v.range_cardinality(..deferred_internal_id) as usize,
None => v.len() as usize,
};
f(k.borrow(), count)
})
}
fn for_each_value_map(
&self,
_hw_counter: &HardwareCounterCell,

View File

@@ -62,15 +62,6 @@ where
self.in_memory_index.for_each_value(f)
}
fn for_each_count_per_value(
&self,
deferred_internal_id: Option<PointOffsetType>,
f: impl FnMut(&N, usize) -> OperationResult<()>,
) -> OperationResult<()> {
self.in_memory_index
.for_each_count_per_value(deferred_internal_id, f)
}
fn for_each_value_map(
&self,
hw_counter: &HardwareCounterCell,

View File

@@ -60,15 +60,6 @@ where
self.in_memory_index.for_each_value(f)
}
fn for_each_count_per_value(
&self,
deferred_internal_id: Option<PointOffsetType>,
f: impl FnMut(&N, usize) -> OperationResult<()>,
) -> OperationResult<()> {
self.in_memory_index
.for_each_count_per_value(deferred_internal_id, f)
}
fn for_each_value_map(
&self,
hw_counter: &HardwareCounterCell,

View File

@@ -8,7 +8,6 @@ use common::counter::iterator_hw_measurement::HwMeasurementIteratorExt;
use common::persisted_hashmap::{Key, READ_ENTRY_OVERHEAD};
use common::types::PointOffsetType;
use common::universal_io::UniversalRead;
use itertools::Itertools;
use super::super::read_ops::MapIndexRead;
use super::super::{IdIter, MapIndexKey};
@@ -159,27 +158,6 @@ impl<'a, N: MapIndexKey + Key + ?Sized + 'a, S: UniversalRead> MapIndexRead<'a,
self.storage.value_to_points.for_each_key(f)
}
fn for_each_count_per_value(
&self,
deferred_internal_id: Option<PointOffsetType>,
mut f: impl FnMut(&N, usize) -> OperationResult<()>,
) -> OperationResult<()> {
self.storage.value_to_points.for_each_entry(|k, v| {
let count = v
.iter()
.filter(|&&idx| {
!self.storage.deleted.get_bit(idx as usize).unwrap_or(true)
// TODO(deferred): Maybe we can improve this filter and use take_while instead. For this we
// need to make sure that `v` is always sorted which we _can_ enforce when finalizing the index.
&& deferred_internal_id.is_none_or(|deferred| idx < deferred)
})
.unique()
.count();
f(k, count)
})
}
fn for_each_value_map(
&self,
hw_counter: &HardwareCounterCell,

View File

@@ -106,24 +106,6 @@ where
}
}
fn for_each_count_per_value(
&self,
deferred_internal_id: Option<PointOffsetType>,
f: impl FnMut(&N, usize) -> OperationResult<()>,
) -> OperationResult<()> {
match self {
ReadOnlyMapIndex::Appendable(index) => {
index.for_each_count_per_value(deferred_internal_id, f)
}
ReadOnlyMapIndex::Immutable(index) => {
index.for_each_count_per_value(deferred_internal_id, f)
}
ReadOnlyMapIndex::OnDisk(index) => {
index.for_each_count_per_value(deferred_internal_id, f)
}
}
}
fn for_each_value_map(
&self,
hw_counter: &HardwareCounterCell,

View File

@@ -56,17 +56,6 @@ pub trait MapIndexRead<'a, N: MapIndexKey + ?Sized + 'a>: Sized {
fn for_each_value(&self, f: impl FnMut(&N) -> OperationResult<()>) -> OperationResult<()>;
/// Iterate `(value, count)` pairs.
///
/// `deferred_internal_id` (mutable / mmap only) restricts the count to
/// point IDs strictly less than the given value. The immutable variant
/// does not support deferred filtering and asserts the argument is `None`.
fn for_each_count_per_value(
&self,
deferred_internal_id: Option<PointOffsetType>,
f: impl FnMut(&N, usize) -> OperationResult<()>,
) -> OperationResult<()>;
fn for_each_value_map(
&self,
hw_counter: &HardwareCounterCell,
@@ -349,24 +338,6 @@ where
}
}
fn for_each_count_per_value(
&self,
deferred_internal_id: Option<PointOffsetType>,
f: impl FnMut(&N, usize) -> OperationResult<()>,
) -> OperationResult<()> {
// The immutable variant does not support deferred filtering — it
// asserts the argument is `None`. Two reasons we don't implement it:
// - We don't have both deferred points and an immutable index.
// - It is not trivial (nor performant) to implement correct filtering
// for this index variant as it doesn't work well in combination
// with the way it handles deletions.
match self {
MapIndex::Mutable(index) => index.for_each_count_per_value(deferred_internal_id, f),
MapIndex::Immutable(index) => index.for_each_count_per_value(deferred_internal_id, f),
MapIndex::OnDisk(index) => index.for_each_count_per_value(deferred_internal_id, f),
}
}
fn for_each_value_map(
&self,
hw_counter: &HardwareCounterCell,

View File

@@ -158,7 +158,7 @@ where
) -> OperationResult<HashMap<FacetValue, usize>> {
let Some(filter) = &request.filter else {
// No filter: count how many visible points each value has.
return self.get_facet_counts(facet_index, None, hw_counter);
return self.get_facet_counts(facet_index, None, is_stopped, hw_counter);
};
let cardinality = self
@@ -216,7 +216,7 @@ where
hw_counter,
)?;
return self.get_facet_counts(facet_index, Some(candidates), hw_counter);
return self.get_facet_counts(facet_index, Some(candidates), is_stopped, hw_counter);
};
let probe = FilterProbe::Lazy {
@@ -305,6 +305,38 @@ where
Ok(hits)
}
/// Count a value's posting points that are visible, and that pass `probe`
/// when set.
///
/// Visibility (deferred + soft-deleted points) is resolved through the id
/// tracker's [`filter_deferred_and_deleted`], the single source of truth for
/// point-offset visibility — the facet index itself stays unaware of how
/// offsets map to deferred/deleted state. Postings are sorted ascending, so
/// adjacent dedup collapses repeats.
///
/// [`filter_deferred_and_deleted`]: crate::id_tracker::point_mappings::PointMappingsRefEnum::filter_deferred_and_deleted
fn count_visible_posting(
&self,
iter: &mut dyn Iterator<Item = PointOffsetType>,
probe: Option<&FilterProbe<'_>>,
) -> usize {
#[cfg(debug_assertions)]
let iter = {
let mut prev_id = None;
iter.inspect(move |&id| {
let previous = prev_id.get_or_insert(id);
debug_assert!(*previous <= id, "Sorted iter assertion broken");
*previous = id;
})
};
self.id_tracker
.point_mappings()
.filter_deferred_and_deleted(iter.dedup(), DeferredBehavior::VisibleOnly)
.filter(|&point_id| probe.is_none_or(|probe| probe.check(point_id)))
.count()
}
/// Iterate every value's posting and count the points passing `probe`.
fn visit_facet_iter<F: FacetIndex>(
&self,
@@ -316,32 +348,12 @@ where
) -> OperationResult<HashMap<FacetValue, usize>> {
let mut hits = HashMap::new();
let max_id = self.deferred_internal_id().unwrap_or(PointOffsetType::MAX);
// Count a value's posting points that are visible and pass the probe.
let count_matching = |iter: &mut dyn Iterator<Item = PointOffsetType>| -> usize {
#[cfg(debug_assertions)]
let iter = {
let mut prev_id = None;
iter.inspect(move |&id| {
let previous = prev_id.get_or_insert(id);
debug_assert!(*previous <= id, "Sorted iter assertion broken");
*previous = id;
})
};
iter.dedup()
.take_while(|&point_id| point_id < max_id)
.filter(|&point_id| probe.check(point_id))
.count()
};
match candidates {
// Sampling: visit only the candidate values' postings.
Some(candidates) => {
facet_index.for_values_map(candidates.into_iter(), hw_counter, |value, iter| {
check_process_stopped(is_stopped)?;
let count = count_matching(iter);
let count = self.count_visible_posting(iter, Some(probe));
if count > 0 {
hits.insert(value, count);
}
@@ -352,7 +364,7 @@ where
None => {
facet_index.for_each_value_map(hw_counter, |value, iter| {
check_process_stopped(is_stopped)?;
let count = count_matching(iter);
let count = self.count_visible_posting(iter, Some(probe));
if count > 0 {
hits.insert(value.to_owned(), count);
}
@@ -364,34 +376,36 @@ where
Ok(hits)
}
/// Count each value's visible points directly from the index postings,
/// without a filter. Visibility (deferred + soft-deleted) is resolved by the
/// id tracker via [`Self::count_visible_posting`].
fn get_facet_counts(
&self,
facet_index: &impl FacetIndex,
candidates: Option<HashSet<FacetValue>>,
is_stopped: &AtomicBool,
hw_counter: &HardwareCounterCell,
) -> Result<HashMap<FacetValue, usize>, OperationError> {
let mut hits = HashMap::new();
let deferred_internal_id = self.deferred_internal_id();
match candidates {
// Sampling: count only the candidate values, looking each up directly.
// Sampling: count only the candidate values, walking each posting.
Some(candidates) => {
facet_index.for_counts_per_value(
candidates.into_iter(),
deferred_internal_id,
hw_counter,
|hit| {
if hit.count > 0 {
hits.insert(hit.value, hit.count);
}
Ok(())
},
)?;
facet_index.for_values_map(candidates.into_iter(), hw_counter, |value, iter| {
check_process_stopped(is_stopped)?;
let count = self.count_visible_posting(iter, None);
if count > 0 {
hits.insert(value, count);
}
Ok(())
})?;
}
// No sampling: count every value in the index.
// Full scan: count every value in the index.
None => {
facet_index.for_each_count_per_value(deferred_internal_id, |hit| {
if hit.count > 0 {
hits.insert(hit.value.to_owned(), hit.count);
facet_index.for_each_value_map(hw_counter, |value, iter| {
check_process_stopped(is_stopped)?;
let count = self.count_visible_posting(iter, None);
if count > 0 {
hits.insert(value.to_owned(), count);
}
Ok(())
})?;
@@ -481,15 +495,22 @@ where
values.extend(iter.map(|v| v.to_owned()));
})?;
} else {
facet_index.for_each_visible_value(
hw_counter,
self.deferred_internal_id(),
|value_ref| {
check_process_stopped(is_stopped)?;
// No filter: keep a value if any of its points is visible. Visibility
// resolution is delegated to the id tracker rather than re-derived
// from the deferred threshold inside the facet index.
facet_index.for_each_value_map(hw_counter, |value_ref, iter| {
check_process_stopped(is_stopped)?;
let has_visible_point = self
.id_tracker
.point_mappings()
.filter_deferred_and_deleted(iter, DeferredBehavior::VisibleOnly)
.next()
.is_some();
if has_visible_point {
values.insert(value_ref.to_owned());
Ok(())
},
)?;
}
Ok(())
})?;
};
Ok(values)