Skip to content

fix(vikingdb): correct inverted branch polarity in genFilter partition filter#2721

Open
nankingjing wants to merge 1 commit into
coze-dev:mainfrom
nankingjing:fix-vikingdb-genfilter-polarity
Open

fix(vikingdb): correct inverted branch polarity in genFilter partition filter#2721
nankingjing wants to merge 1 commit into
coze-dev:mainfrom
nankingjing:fix-vikingdb-genfilter-polarity

Conversation

@nankingjing

Copy link
Copy Markdown

Problem

In backend/infra/document/searchstore/impl/vikingdb/vikingdb_searchstore.go, the genFilter function combines a DSL scope filter with the partition filter. The branch polarity is inverted:

op := map[string]any{"op": "must", "field": key, "conds": conds}

if filter != nil {
    filter = op                       // overwrites & DROPS the DSL scope filter
} else {
    filter = map[string]any{
        "op":    "and",
        "conds": []map[string]any{op, filter},  // filter is nil here -> nil embedded in conds
    }
}

Consequences:

  • When a DSL scope filter exists (filter != nil), filter = op silently discards it, keeping only the partition filter. Retrieval then ignores the caller's scope constraints, returning documents that should have been filtered out.
  • When there is no DSL filter (filter == nil), the else branch builds {"op":"and","conds":[op, filter]} where filter is nil, embedding a nil map into the conds slice.

Fix

Swap the condition so the branch bodies match their intent. The bodies are already correct for the swapped condition, so this is a minimal one-character change:

if filter == nil {
    filter = op                       // no DSL filter -> just the partition op
} else {
    filter = map[string]any{          // DSL filter present -> AND the two together
        "op":    "and",
        "conds": []map[string]any{op, filter},
    }
}

Now:

  • No DSL filter -> filter = op (partition-only), no nil embedded.
  • DSL filter present -> {op:and, conds:[op, filter]}, preserving both constraints.

Verification

Verified by reading the source; not executed (no Go toolchain available in this environment). The change is a +1/-1 diff limited to the single inverted condition; the unrelated correct usage of if filter != nil at the call site (SetFilter) is untouched.

@nankingjing

Copy link
Copy Markdown
Author

Review: fix-vikingdb-genfilter-polarity (#2721)

Verdict: LGTM

Classic inverted boolean condition. The genFilter function builds a composite filter by iterating over partitions. When building the first partition filter, filter is nil and should be set directly. When adding subsequent partitions, filter already exists and should be wrapped in an AND clause with the new condition.

The old code had if filter != nil (inverted): it would overwrite an existing filter instead of combining it, and try to AND with a nil filter on the first iteration. The fix corrects the condition to if filter == nil, giving the correct behavior for both the first and subsequent iterations.

No issues found. Merge-ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant