perf(contains-pr12): batch eligible Contains filters by default - #12999
Draft
aliszka wants to merge 3 commits into
Draft
perf(contains-pr12): batch eligible Contains filters by default#12999aliszka wants to merge 3 commits into
aliszka wants to merge 3 commits into
Conversation
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>
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Secrets | View in Orca |
|
Contributor
There was a problem hiding this comment.
🟡 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
buildAllowListfalls 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 nocontains_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) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Batched resolution of
ContainsAny/ContainsAll/ContainsNonehas shipped behindQUERY_BATCHED_CONTAINS_ENABLEDsince v1.38.8, defaulting off. This makes it the default, keeps the variable as the way out, and lets alen(prop)filter batch too.Motivation
A deployment got the faster path only by knowing to ask for it. Both paths answer identically, which
TestDocIDs_BatchedMatchesDesugaredpins, 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_enabledoverride 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.FromEnvwas 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
Getanswers false for a nil one, so a construction site one forgotten option short would take the slow path in silence. A gate holdingtrueis 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.ValidateFiltersadmits 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 shapereadFromBucketcannot 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 deliberatesearcher.go:classifyContainsBatchLength— the new family; one int builder now names either bucketshard_write_batch_delete.go— the slow-query install, ungated like the four sites inindex.gocontainsany_docids_bench_test.go— every differential row now fails unless the batched path ranRisks
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