mirror of
https://github.com/milvus-io/milvus.git
synced 2026-07-21 10:15:43 +00:00
enhance: pin scalar index only after committing to ScalarIndex exec path (#49238)
issue: #49237 ## Summary `SegmentExpr::InitSegmentExpr()` was unconditionally pinning the scalar index via `PinIndex()` in its ctor. On a tiered-storage `ChunkedSegmentSealedImpl`, that triggers `CacheSlot::PinCells()` and a cold fetch even for expressions whose exec path is `TextIndex` / `PkIndex` / `JsonStats` / `RawData` — none of which ever touch `pinned_index_`. Observed impact: `TEXT_MATCH(field, "…")` on an `enable_match=true` VARCHAR field slows down after an `INVERTED` AUTOINDEX is added on the same field, because every expr construction cold-loads the scalar index cell that `TEXT_MATCH` never reads. This PR restructures `SegmentExpr::DetermineExecPath()` so that `EnsurePinnedIndex()` runs only once we've fully committed to `ExprExecPath::ScalarIndex`. Pin happens **iff** `exec_path_ == ScalarIndex`. ## Changes - `InitSegmentExpr()` sets `num_index_chunk_` speculatively from `segment_->HasIndex()` (cheap bitset, no pin), so `HasCompatibleScalarIndex()` can decide existence without triggering a cold fetch. - New `SegmentInterface::GetJsonFlatIndexNestedPath()` exposes the `JsonFlatIndex` nested path already stored in segment JSON-index metadata; `SegmentSealed` overrides it with a mirror of `PinJsonIndex`'s prefix-matching loop, but without pinning. - `IsJsonPathCompatible()` is extracted from `HasCompatibleScalarIndex()` and rewritten to use the new segment API, removing its dependency on `pinned_index_`. - Base `DetermineExecPath()` short-circuits to `RawData` when either check fails and only then calls `EnsurePinnedIndex()`. A post-pin reconciliation guard also falls back to `RawData` if `HasIndex()` was speculatively true but `scalar_indexings_` had no entry (rare edge case: vector/binlog-index-only field or a load transient). All subclass `DetermineExecPath` short-circuits (`TextMatch` / `PhraseMatch` / `PkIndex` / `JsonStats`) already return before reaching the base, so they still never pin. Downstream `pinned_index_[i]` accessors are gated on `exec_path_ == ScalarIndex` (either explicitly or via `UseIndexCursor()` / `ProcessIndexChunks` helpers), so the invariant is preserved without changing dispatch sites. Observable behavior of `HasCompatibleScalarIndex()` and `CanUseNestedIndex()` is unchanged: the JSON-path narrowing moves into `IsJsonPathCompatible()` but the combined decision in `DetermineExecPath` is identical, and `CanUseNestedIndex()`'s `pinned_index_.empty()` guard keeps the external return value the same for every valid state. ## Test plan - [ ] `make core` + segcore C++ UT (`tests/cpp_test/`), especially expression-related suites - [ ] `make test-querynode` / `make test-proxy` - [ ] Manual: with tiered storage / lazy loading on QueryNode, compare `TEXT_MATCH(field, "...")` P99 latency on a collection that has `enable_match=true` VARCHAR + AUTOINDEX `INVERTED` on the same field — expect no cold-fetch of the scalar INVERTED cell per query - [ ] Regression: scalar `==` / `IN` / range / `LIKE` / JSON-path / PK queries still use the correct index path (`exec_path_ == ScalarIndex` → `pinned_index_` populated) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Shawn Wang <shawn.wang@zilliz.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
sparknack
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
fe13e4cb06
commit
693eb2a265
@@ -225,16 +225,17 @@ class SegmentExpr : public Expr {
|
||||
pk_type_ = field_meta.get_data_type();
|
||||
}
|
||||
|
||||
pinned_index_ = PinIndex(op_ctx_,
|
||||
segment_,
|
||||
field_meta,
|
||||
nested_path_,
|
||||
value_type_,
|
||||
allow_any_json_cast_type_,
|
||||
is_json_contains_);
|
||||
if (pinned_index_.size() > 0) {
|
||||
num_index_chunk_ = pinned_index_.size();
|
||||
}
|
||||
// Scalar index is pinned lazily by EnsurePinnedIndex() when (and only
|
||||
// when) DetermineExecPath() commits to ExprExecPath::ScalarIndex.
|
||||
// num_index_chunk_ stays 0 here and is set to pinned_index_.size()
|
||||
// inside EnsurePinnedIndex(), so the invariant
|
||||
// num_index_chunk_ == pinned_index_.size()
|
||||
// always holds. Pre-pin existence checks go through
|
||||
// HasCompatibleScalarIndex(), which asks the segment directly and
|
||||
// does not require a pin -- so short-circuit exec paths
|
||||
// (TextIndex/PkIndex/JsonStats) and the RawData path never pay for a
|
||||
// PinCells() cold fetch under tiered storage.
|
||||
|
||||
// if index not include raw data, also need load data
|
||||
if (segment_->HasFieldData(field_id_)) {
|
||||
if (segment_->is_chunked()) {
|
||||
@@ -245,6 +246,29 @@ class SegmentExpr : public Expr {
|
||||
}
|
||||
}
|
||||
|
||||
// Pin the scalar index cell. Called by DetermineExecPath() only after the
|
||||
// expression has committed to ExprExecPath::ScalarIndex, so the pin's
|
||||
// lifetime matches real usage: short-circuit paths
|
||||
// (TextIndex/PkIndex/JsonStats) and the RawData path never call it and
|
||||
// the scalar index cell stays cold in tiered storage. Idempotent.
|
||||
void
|
||||
EnsurePinnedIndex() {
|
||||
if (pinned_index_initialized_) {
|
||||
return;
|
||||
}
|
||||
pinned_index_initialized_ = true;
|
||||
auto& schema = segment_->get_schema();
|
||||
auto& field_meta = schema[field_id_];
|
||||
pinned_index_ = PinIndex(op_ctx_,
|
||||
segment_,
|
||||
field_meta,
|
||||
nested_path_,
|
||||
value_type_,
|
||||
allow_any_json_cast_type_,
|
||||
is_json_contains_);
|
||||
num_index_chunk_ = pinned_index_.size();
|
||||
}
|
||||
|
||||
virtual bool
|
||||
IsSource() const override {
|
||||
return true;
|
||||
@@ -1926,37 +1950,58 @@ class SegmentExpr : public Expr {
|
||||
// Only called internally by DetermineExecPath() and CanUseNestedIndex().
|
||||
bool
|
||||
HasCompatibleScalarIndex() const {
|
||||
// Queries segment metadata directly -- no pin required, so short-
|
||||
// circuit exec paths and the RawData fallback skip the cold fetch.
|
||||
// JSON-specific path compatibility is handled by the separate
|
||||
// IsJsonPathCompatible() helper, which also avoids pinning.
|
||||
// Ngram index should be used in specific execution path (CanUseNgramIndex -> ExecNgramMatch).
|
||||
// TODO: if multiple indexes are supported, this logic should be changed
|
||||
if (num_index_chunk_ == 0 || CanUseNgramIndex()) {
|
||||
return false;
|
||||
}
|
||||
//
|
||||
// JSON indexes are tracked separately from scalar/vector/binlog
|
||||
// indexes (see SegmentInterface::HasJsonIndex), so dispatch by
|
||||
// field type to avoid widening HasIndex() semantics -- which
|
||||
// ReorderConjunctExpr and other callers rely on remaining narrow.
|
||||
bool has = (field_type_ == DataType::JSON)
|
||||
? segment_->HasJsonIndex(field_id_)
|
||||
: segment_->HasIndex(field_id_);
|
||||
return has && !CanUseNgramIndex();
|
||||
}
|
||||
|
||||
// JSON fields only: verify that a JsonFlatIndex exists for the expr's
|
||||
// nested path and that prefix matching is valid. Reads segment-level
|
||||
// JSON index metadata via GetJsonFlatIndexNestedPath() -- does NOT pin
|
||||
// the index cell, so callers can use this to skip ScalarIndex
|
||||
// commitment (and its associated cold fetch) before paying for
|
||||
// EnsurePinnedIndex(). Returns true for non-JSON fields and for JSON
|
||||
// fields without a JsonFlatIndex covering the query path.
|
||||
bool
|
||||
IsJsonPathCompatible() const {
|
||||
// For JSON fields with JsonFlatIndex, check if prefix matching is valid.
|
||||
// Tantivy JSON index can handle nested object paths (e.g., "a.b") but NOT
|
||||
// numeric array indices (e.g., "a.0"). Per RFC 6901, JSON Pointer doesn't
|
||||
// distinguish between array indices and object keys syntactically. Since
|
||||
// Tantivy doesn't store array index information, we must fall back to
|
||||
// brute-force search when the relative path contains numeric segments.
|
||||
if (field_type_ != DataType::JSON || pinned_index_.empty()) {
|
||||
if (field_type_ != DataType::JSON) {
|
||||
return true;
|
||||
}
|
||||
|
||||
auto json_flat_index =
|
||||
dynamic_cast<const index::JsonFlatIndex*>(pinned_index_[0].get());
|
||||
if (json_flat_index == nullptr) {
|
||||
return true;
|
||||
}
|
||||
|
||||
auto index_path = json_flat_index->GetNestedPath();
|
||||
auto query_path = milvus::Json::pointer(nested_path_);
|
||||
auto index_path =
|
||||
segment_->GetJsonFlatIndexNestedPath(field_id_, query_path);
|
||||
if (index_path.empty()) {
|
||||
// No JsonFlatIndex covers this path; nothing JSON-specific to
|
||||
// reject here. The caller will decide between ScalarIndex and
|
||||
// RawData based on HasCompatibleScalarIndex() alone.
|
||||
return true;
|
||||
}
|
||||
|
||||
// Exact match - safe to use index
|
||||
if (index_path == query_path) {
|
||||
return true;
|
||||
}
|
||||
|
||||
// PinJsonIndex guarantees index_path is a prefix of query_path
|
||||
// GetJsonFlatIndexNestedPath guarantees index_path is a prefix of
|
||||
// query_path.
|
||||
|
||||
// Get relative path (e.g., if index_path="/a" and query_path="/a/0/b",
|
||||
// relative_path="/0/b")
|
||||
@@ -2092,10 +2137,29 @@ class SegmentExpr : public Expr {
|
||||
// Determine the execution path for this expression.
|
||||
// Called during initialization by subclass constructors.
|
||||
// Subclasses should override to implement operator-specific logic.
|
||||
// The scalar index is pinned only when we commit to the ScalarIndex
|
||||
// path. JSON path compatibility is checked via segment-level metadata
|
||||
// (GetJsonFlatIndexNestedPath) before any pin, so an incompatible
|
||||
// nested path falls back to RawData without ever touching the cache
|
||||
// slot. Short-circuit subclass paths (TextIndex/PkIndex/JsonStats)
|
||||
// bypass this method entirely and never pin either.
|
||||
virtual void
|
||||
DetermineExecPath() {
|
||||
exec_path_ = HasCompatibleScalarIndex() ? ExprExecPath::ScalarIndex
|
||||
: ExprExecPath::RawData;
|
||||
if (!HasCompatibleScalarIndex() || !IsJsonPathCompatible()) {
|
||||
exec_path_ = ExprExecPath::RawData;
|
||||
return;
|
||||
}
|
||||
EnsurePinnedIndex();
|
||||
// HasCompatibleScalarIndex() queries HasIndex(), which can return
|
||||
// true for a vector/binlog-index-only field or a mid-load state
|
||||
// where PinIndex() still yields nothing. Fall back to RawData when
|
||||
// the pin actually came up empty so pinned_index_ is non-empty iff
|
||||
// exec_path_ == ScalarIndex holds.
|
||||
if (pinned_index_.empty()) {
|
||||
exec_path_ = ExprExecPath::RawData;
|
||||
return;
|
||||
}
|
||||
exec_path_ = ExprExecPath::ScalarIndex;
|
||||
}
|
||||
|
||||
// Slice cached result bitmap for the current batch.
|
||||
@@ -2178,7 +2242,13 @@ class SegmentExpr : public Expr {
|
||||
bool execute_all_at_once_{false};
|
||||
// used for reducing cache miss latency in tiered storage
|
||||
bool prefetched_{false};
|
||||
// Scalar index is pinned lazily by EnsurePinnedIndex(). Pre-pin
|
||||
// existence checks (HasCompatibleScalarIndex) query segment metadata
|
||||
// directly, so expressions on short-circuit paths (TextIndex, PkIndex,
|
||||
// JsonStats) and RawData never force a PinCells() cold fetch. After
|
||||
// the pin, num_index_chunk_ == pinned_index_.size() always.
|
||||
std::vector<PinWrapper<const index::IndexBase*>> pinned_index_{};
|
||||
bool pinned_index_initialized_{false};
|
||||
|
||||
int64_t active_count_{0};
|
||||
int64_t num_data_chunk_{0};
|
||||
|
||||
@@ -3267,6 +3267,22 @@ ChunkedSegmentSealedImpl::HasIndex(FieldId field_id) const {
|
||||
get_bit(binlog_index_bitset_, field_id);
|
||||
}
|
||||
|
||||
bool
|
||||
ChunkedSegmentSealedImpl::HasJsonIndex(FieldId field_id) const {
|
||||
// JSON indexes (JsonFlatIndex + JSON-cast) live in a separate per-segment
|
||||
// vector rather than in either index bitset. Kept as a distinct API so
|
||||
// HasIndex() preserves its narrower "scalar/vector/binlog index exists"
|
||||
// semantics for ReorderConjunctExpr and other consumers.
|
||||
return json_indices.withRLock([&](const auto& vec) {
|
||||
for (const auto& index : vec) {
|
||||
if (index.field_id == field_id) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
return false;
|
||||
});
|
||||
}
|
||||
|
||||
bool
|
||||
ChunkedSegmentSealedImpl::HasFieldData(FieldId field_id) const {
|
||||
std::shared_lock lck(mutex_);
|
||||
|
||||
@@ -125,6 +125,8 @@ class ChunkedSegmentSealedImpl : public SegmentSealed {
|
||||
bool
|
||||
HasIndex(FieldId field_id) const override;
|
||||
bool
|
||||
HasJsonIndex(FieldId field_id) const override;
|
||||
bool
|
||||
HasFieldData(FieldId field_id) const override;
|
||||
|
||||
std::pair<std::shared_ptr<ChunkedColumnInterface>, bool>
|
||||
|
||||
@@ -215,6 +215,18 @@ class SegmentInterface {
|
||||
return {};
|
||||
}
|
||||
|
||||
// Returns the nested path of the JsonFlatIndex that would match
|
||||
// query_path for field_id, or empty string if no JsonFlatIndex covers
|
||||
// this path. Reads segment-level JSON index metadata directly -- does
|
||||
// NOT pin the index cell. Used by expression DetermineExecPath() to
|
||||
// check path compatibility without triggering a tiered-storage cold
|
||||
// fetch when the decision is going to be RawData anyway.
|
||||
virtual std::string
|
||||
GetJsonFlatIndexNestedPath(FieldId field_id,
|
||||
std::string_view query_path) const {
|
||||
return "";
|
||||
}
|
||||
|
||||
virtual std::vector<PinWrapper<const index::IndexBase*>>
|
||||
PinIndex(milvus::OpContext* op_ctx,
|
||||
FieldId field_id,
|
||||
@@ -435,6 +447,16 @@ class SegmentInternalInterface : public SegmentInterface {
|
||||
virtual bool
|
||||
HasIndex(FieldId field_id) const = 0;
|
||||
|
||||
// JSON indexes (JsonFlatIndex + JSON-cast scalar) live in a separate
|
||||
// per-segment container from the scalar/vector/binlog index bitsets, so
|
||||
// they are checked via this dedicated API rather than widening HasIndex().
|
||||
// Default returns false for segment types that never build JSON indexes
|
||||
// (e.g. growing segments); sealed segments override.
|
||||
virtual bool
|
||||
HasJsonIndex(FieldId field_id) const {
|
||||
return false;
|
||||
}
|
||||
|
||||
int64_t
|
||||
get_real_count() const override;
|
||||
|
||||
|
||||
@@ -129,6 +129,46 @@ class SegmentSealed : public SegmentInternalInterface {
|
||||
return {res};
|
||||
}
|
||||
|
||||
// Mirror of the JsonFlatIndex prefix-matching loop in PinJsonIndex(),
|
||||
// but only reads the nested_path metadata and returns it -- does not
|
||||
// touch the CacheSlot or pin any cell. Callers can use this to decide
|
||||
// whether a JSON query is compatible with the indexed path before
|
||||
// committing to the ScalarIndex exec path (and paying for a pin).
|
||||
std::string
|
||||
GetJsonFlatIndexNestedPath(FieldId field_id,
|
||||
std::string_view query_path) const override {
|
||||
return json_indices.withRLock([&](auto& vec) -> std::string {
|
||||
std::string best_path;
|
||||
int path_len_diff = std::numeric_limits<int>::max();
|
||||
for (const auto& index : vec) {
|
||||
if (index.field_id != field_id) {
|
||||
continue;
|
||||
}
|
||||
if (index.cast_type.data_type() !=
|
||||
JsonCastType::DataType::JSON) {
|
||||
continue;
|
||||
}
|
||||
if (query_path.length() < index.nested_path.length()) {
|
||||
continue;
|
||||
}
|
||||
if (query_path.substr(0, index.nested_path.length()) !=
|
||||
index.nested_path) {
|
||||
continue;
|
||||
}
|
||||
int current_len_diff =
|
||||
query_path.length() - index.nested_path.length();
|
||||
if (current_len_diff < path_len_diff) {
|
||||
path_len_diff = current_len_diff;
|
||||
best_path = index.nested_path;
|
||||
}
|
||||
if (path_len_diff == 0) {
|
||||
break;
|
||||
}
|
||||
}
|
||||
return best_path;
|
||||
});
|
||||
}
|
||||
|
||||
virtual PinWrapper<index::NgramInvertedIndex*>
|
||||
GetNgramIndex(milvus::OpContext* op_ctx,
|
||||
FieldId field_id) const override = 0;
|
||||
|
||||
Reference in New Issue
Block a user