Skip to content

perf(contains-pr12): batch eligible Contains filters by default - #12999

Draft
aliszka wants to merge 3 commits into
stable/v1.38from
opt/contains-pr12-ungate
Draft

perf(contains-pr12): batch eligible Contains filters by default#12999
aliszka wants to merge 3 commits into
stable/v1.38from
opt/contains-pr12-ungate

Conversation

@aliszka

@aliszka aliszka commented Sep 8, 2026

Copy link
Copy Markdown
Member

Batched resolution of ContainsAny/ContainsAll/ContainsNone has shipped behind QUERY_BATCHED_CONTAINS_ENABLED since v1.38.8, defaulting off. This makes it the default, keeps the variable as the way out, and lets a len(prop) filter batch too.

Motivation

A deployment got the faster path only by knowing to ask for it. Both paths answer identically, which TestDocIDs_BatchedMatchesDesugared pins, so the default was costing everyone who never set the variable.

Approach

The variable keeps its name and its meaning; only the value an unset one implies changes. Both it and the query_batched_contains_enabled override key are released surface, and the key is decoded by reflection straight into its field, so inverting or renaming either would silently drop an existing setting. FromEnv was separately discarding a config-file value by assigning unconditionally; it now leaves the file's value alone unless the variable carries something, making precedence variable, then file, then default — an empty variable counting as unset.

The gate is read through a helper because Get answers false for a nil one, so a construction site one forgotten option short would take the slow path in silence. A gate holding true is not a no-op either: the overrides manager writes through the pointer it registered, so only a searcher holding it honours a later change.

A len(prop) filter batches, and no supported API can send one. ValidateFilters admits only (not) equal and greater/less than (equal) on a length path, so the arm is groundwork and an acceptance case pins the refusal. A property whose own name ends in the length suffix is a different shape readFromBucket cannot distinguish, deciding from the suffix alone; the classifier applies the same test rather than letting batching answer a filter every released version refuses.

Key areas for review

  • searcher.go:classifyContainsBatch — the length-suffix guard, and why declining is deliberate
  • searcher.go:classifyContainsBatchLength — the new family; one int builder now names either bucket
  • shard_write_batch_delete.go — the slow-query install, ungated like the four sites in index.go
  • containsany_docids_bench_test.go — every differential row now fails unless the batched path ran

Risks

  • The default changes for every deployment. Three ways out — the variable, the config file, the runtime overrides key — of which only the last takes effect without a restart.
  • The slow-query install on batch delete is not gated on the log being enabled, matching the four sites in index.go; it costs about one allocation per desugared leaf either way.

Testing

The differential test grows a length arm, and each row now resolves through a helper that fails unless the batched path ran. The acceptance suite covers the default on, a second instance with the variable off, batch delete, and a filtered vector search; the last two are routes nothing else drives. Every new assertion was mutation-tested.

chaos: https://github.com/weaviate/weaviate-chaos-engineering/actions/runs/34327297507
e2e: https://github.com/weaviate/weaviate-e2e-tests/actions/runs/34327207624

aliszka and others added 3 commits September 8, 2026 15:18
The batched resolution shipped opt-in, so a Contains filter took the
per-value desugared path unless someone knew to ask for the faster one.
Both paths answer identically — a differential test pins that — so the
default was costing every deployment that never set the variable.

QUERY_BATCHED_CONTAINS_ENABLED and the query_batched_contains_enabled
override key keep their names and their meaning; only the value an unset
variable implies changes. Both have shipped since v1.38.8, and the
override key is decoded by reflection straight into its field, so
inverting either one would have silently dropped an existing setting or
reversed what it means.

FromEnv assigned the field unconditionally, and LoadConfig decodes the
config file before calling it, so a deployment holding
query_batched_contains_enabled: false in its config file got true. It now
assigns only when the variable carries a value, or when nothing has set
the field at all, leaving the file's value alone. Precedence is the one
the write order already implied: variable, then file, then the default.
An empty variable counts as unset, so a values file expressing "leave
this alone" as an empty string does not disable the feature.

The gate reads through batchedContainsEnabledOrDefault, because Get
answers false for a nil gate and every construction site would otherwise
be one forgotten option away from a silently slower query. That is the
same shape as GrpcWebEnabledOrDefault. A gate holding true is not a
no-op: the overrides manager writes through the pointer it registered, so
only a searcher holding that pointer honours a later change.

The searcher tests drop their explicit enable, so they exercise the
unwired-gate default; the production wiring is covered end to end by the
acceptance run, which leaves the variable unset and asserts no filter
recorded a desugar reason. A second instance runs with the variable set
to false and asserts the opposite, so the escape hatch is covered too.

Batch delete resolves its filter through a searcher of its own and had no
slow-query context, so nothing recorded which resolver produced the set
it deleted. Shard.FindUUIDs now installs one and reports through the
shard's reporter, matching the four search paths in index.go. Like those
four, the install is not gated on the slow log being enabled: it costs
about one allocation and 55 bytes per desugared leaf, paid whether or not
the log is on. Consistency with those sites was preferred to gating this
one; the cost falls on the path this change exists to make rarer.

A filtered vector search is the only caller of Shard.buildAllowList and
no case reached it, so the acceptance corpus carries vectors and one
nearVector query filters through it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A length filter was the one shape the classifier turned away for a reason
that was not about the data: lengths land in a roaringset bucket, encoded
by the same lexicographically sortable int64 the primitive int family
uses, so a length batch differs from an int batch only in which bucket it
names. Batching it removes a family from the desugared path without
changing what any query answers.

No supported API can send this shape today. ValidateFilters admits only
(not) equal and greater/less than (equal) on a length path, so a len()
Contains filter is refused before it reaches the searcher; the arm becomes
reachable when that whitelist widens. An acceptance case pins the refusal,
so widening it fails there until the shape gets coverage of its own.

The leaf therefore has to carry the length property name rather than the
property's own. newBatchedContainsPair took the property and derived the
name itself, which left every family sharing one rule; it now takes the
name, so each builder states the bucket it reads and the length builder
cannot inherit the wrong one. One int builder serves both, since only that
name differs.

Bucket presence is not enough to accept the filter. The desugared path
refuses a length filter while the class does not index lengths, so a batch
reading the bucket directly would answer where that path fails, and the two
would disagree on the same query against the same data. The classifier
checks the flag, so the filter desugars and reaches the existing refusal.

A property whose own name ends in the length suffix is not a length filter,
and readFromBucket cannot tell the two apart: it decides from the suffix
alone. The classifier applies the same test, so batching does not start
answering a filter every released version refuses. That refusal is itself
wrong — the property's own bucket holds the values asked for — and fixing
the suffix test belongs to the path that owns it.

The differential test grows a length arm: several present lengths, every
present length, a duplicated one, present-with-absent and all-absent,
across all three operators. A document holds one length, so ContainsAll
over two distinct lengths is empty by construction and a duplicated
length is the only non-empty ContainsAll — both compared against the
desugared compound. Pointing the length builder at the value bucket
fails that arm and the unit test. Each row resolves through a helper that
fails unless the batched path ran, so an arm that stops batching fails
instead of comparing the desugared path with itself.

Two fixture classes gain an InvertedIndexConfig. Every class reaching the
searcher through a creation path has one, and the property-length and
null-state guards in readFromBucket dereference it without checking, so a
fixture without one crashes the moment a test filters on a length. No load
path fills it, which the classifier declines for rather than dereferencing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
queryIDsErr formatted resp.Errors with %v, and that slice holds pointers, so
every failure read "graphql errors: [0x327b3101a080]". A caller asserting on
the message text had nothing to match.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@aliszka
aliszka requested a balanced review from Copilot September 8, 2026 14:18

@orca-security-eu orca-security-eu Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca

@sonarqubecloud

sonarqubecloud Bot commented Sep 8, 2026

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Two new route-specific acceptance cases do not verify that the batched resolver actually ran.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Makes batched ContainsAny/ContainsAll/ContainsNone resolution the default while preserving configuration opt-out paths.

Changes:

  • Defaults batching on while preserving environment, file, and runtime overrides.
  • Adds batched property-length handling and batch-delete slow-query instrumentation.
  • Expands unit, integration, acceptance, and differential coverage.
File summaries
File Description
usecases/config/environment.go Applies configuration precedence and default-on behavior.
usecases/config/environment_test.go Tests environment and config-file precedence.
usecases/config/config_handler.go Updates configuration documentation.
test/acceptance_with_go_client/batched_contains_tests/batched_contains_test.go Expands end-to-end coverage and logging helpers.
adapters/repos/db/shard_write_batch_delete.go Adds slow-query details to batch-delete filtering.
adapters/repos/db/inverted/searcher.go Implements default-on gating and length batching.
adapters/repos/db/inverted/searcher_integration_test.go Exercises implicit default-on behavior.
adapters/repos/db/inverted/searcher_contains_batch_test.go Tests gating, classification, and length buckets.
adapters/repos/db/inverted/prop_value_pairs_nested_integration_test.go Uses the default batching gate in nested tests.
adapters/repos/db/inverted/filters_integration_test.go Enables property-length indexes in fixtures.
adapters/repos/db/inverted/containsany_docids_bench_test.go Adds differential length coverage and path assertions.
adapters/repos/db/inverted/containsall_bigrows_bench_test.go Uses default-on batching in benchmarks.
adapters/repos/db/aggregator/aggregator.go Documents default-on propagation semantics.
Review details

Suppressed comments (1)

test/acceptance_with_go_client/batched_contains_tests/batched_contains_test.go:412

  • This validates only the result, which is intentionally identical when buildAllowList falls back to desugaring. Consequently the test still passes if this route never exercises the new default batched path. Capture the log delta for this request and assert a fold annotation (and no contains_desugared) so the route-specific coverage matches the test's stated purpose.
		require.ElementsMatch(t,
			[]string{idOf(0), idOf(1), idOf(2)},
			acceptance_with_go_client.GetIds(t, resp, className),
			"the vector search must return exactly what the filter matched")
  • Files reviewed: 13/13 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.


// The query names itself in the slow-query log, so a delete records
// which resolver produced its set.
weaviate.logsMatching(t, ctx, findUUIDsQueryRE)
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.

2 participants