fix(v3): guard empty-bloom needle in eql_v3.matches (CIP-3606) - #421
Merged
freshtonic merged 2 commits intoJul 27, 2026
Conversation
`eql_v3.matches` (the `@@` fuzzy-match operator on the text match/search
domains) reduces to bloom array-containment `match_term(a) @> match_term(b)`.
An empty needle bloom (`{}`) is contained by every value, so a search term
with no n-gram tokens (a sub-trigram string below the tokeniser floor)
matched every row in the table, silently.
The codegen now renders the `@@` wrapper with an empty-needle guard giving
`LIKE`-shaped semantics — an empty needle matches only a value whose own
bloom is also empty (`'' LIKE ''` is true; `'catty' LIKE ''` is false):
SELECT match_term(a) @> match_term(b)
AND (cardinality(match_term(b)) > 0 OR cardinality(match_term(a)) = 0)
The guard is inline (not a helper/CASE) so the top-level `match_term(col)
@> needle` conjunct stays an indexable qualifier and the functional GIN
index still engages for real queries (`col @@ $1`, or a literal). The
wrapper is now non-STRICT: PostgreSQL will not inline a STRICT SQL function
whose body carries the guard's AND/OR, and the body propagates NULL on a
NULL operand on its own, so dropping STRICT preserves NULL semantics while
restoring inlinability. A needle supplied as an uncorrelated subquery no
longer inlines (the guard references the needle twice) and falls back to a
seq scan; parameters and literals — the normal shapes — are unaffected.
Tests: new real-ciphertext fixture `v3_text_empty_bloom` (a 2-char value
encrypts to a genuine `bf: []`), four behavioural tests covering the LIKE
truth table plus a premise guard, index tests switched to literal needles,
and the pre-existing `text_smoke` test that had encoded the bug as intended
("every filter must match the empty needle") corrected to the new
semantics. Adds a changeset and upgrade note U-011.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthrough
ChangesEmpty-bloom match guard
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant QueryPlanner
participant eql_v3_matches
participant match_term
participant GINIndex
QueryPlanner->>GINIndex: Apply match_term(column) @> match_term(needle)
QueryPlanner->>eql_v3_matches: Evaluate guarded match
eql_v3_matches->>match_term: Compute bloom term cardinalities
match_term-->>eql_v3_matches: Return term sets
eql_v3_matches-->>QueryPlanner: Return match result
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/eql-codegen/src/generate.rs (1)
1533-1562: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every generated
@@overload in this regression test.At Line 1544, the assertions cover only the public/public signature. Add equivalent guard and non-
STRICTassertions for the public/jsonb and jsonb/public overloads so a regression in either generated signature cannot reintroduce the empty-needle bug while this test remains green.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/eql-codegen/src/generate.rs` around lines 1533 - 1562, Extend match_domain_renders_matches_wrapper_and_at_at_operator to assert the empty-needle guard and non-STRICT SQL declaration for both public/jsonb and jsonb/public eql_v3.matches overloads, alongside the existing public/public checks. Ensure each overload’s generated body contains the guarded containment expression and each declaration lacks STRICT..changeset/empty-bloom-match-guard.md (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an actual link for the U-011 reference.
The upgrade-note reference is plain text ("See upgrade note U-011"). Since
CHANGELOG.mdis auto-generated from this file and must not be hand-edited afterward, any clickable cross-link needs to be embedded directly in this changeset body to propagate into the generated changelog.📝 Suggested link
-...as before. See upgrade note U-011. +...as before. See upgrade note [U-011](../docs/upgrading/v3.0.md#u-011-empty-bloom-needle-no-longer-matches-every-row).Based on learnings, "If you need a cross-link to appear in the generated changelog (e.g., link to an upgrade note like
docs/upgrading/v3.0.md#u-00X-...), add that link directly to the relevant.changeset/*.mdentry body so it propagates into the generated changelog."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/empty-bloom-match-guard.md at line 5, Update the changeset body’s “See upgrade note U-011” reference to include a clickable link targeting the U-011 section in the upgrade documentation. Keep the existing explanation unchanged and embed the link directly in this changeset so it propagates to the generated changelog.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.changeset/empty-bloom-match-guard.md:
- Line 5: Update the changeset body’s “See upgrade note U-011” reference to
include a clickable link targeting the U-011 section in the upgrade
documentation. Keep the existing explanation unchanged and embed the link
directly in this changeset so it propagates to the generated changelog.
In `@crates/eql-codegen/src/generate.rs`:
- Around line 1533-1562: Extend
match_domain_renders_matches_wrapper_and_at_at_operator to assert the
empty-needle guard and non-STRICT SQL declaration for both public/jsonb and
jsonb/public eql_v3.matches overloads, alongside the existing public/public
checks. Ensure each overload’s generated body contains the guarded containment
expression and each declaration lacks STRICT.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd5345c3-0c10-4e5a-911d-87266e0553bd
📒 Files selected for processing (18)
.changeset/empty-bloom-match-guard.md.gitignorecrates/eql-codegen/src/context.rscrates/eql-codegen/src/generate.rscrates/eql-codegen/src/operator_surface.rscrates/eql-codegen/templates/functions/wrapper.sql.j2docs/upgrading/v3.0.mdsrc/v3/scalars/text/query_text_match_functions.sqlsrc/v3/scalars/text/query_text_search_functions.sqlsrc/v3/scalars/text/query_text_search_ore_functions.sqlsrc/v3/scalars/text/text_match_functions.sqlsrc/v3/scalars/text/text_search_functions.sqlsrc/v3/scalars/text/text_search_ore_functions.sqltests/sqlx/src/fixtures/mod.rstests/sqlx/src/fixtures/v3_text_empty_bloom.rstests/sqlx/tests/encrypted_domain/text/text_match.rstests/sqlx/tests/encrypted_domain/text/text_smoke.rstests/sqlx/tests/generate_all_fixtures.rs
CI was red on two gates: - SQL doc validation / public_identifiers: nine comments referenced the private tracker id CIP-3606. Describe the empty-bloom-needle guard behaviour directly instead. Also repair two comments left with dangling punctuation by an earlier scrub of the same id. - Rust workspace crates / cargo fmt --check: collapse a multi-line assert! in text_match.rs to the one-line form rustfmt wants.
Contributor
|
Review by Codex using GPT-5.6 Sol: The empty-bloom guard is consistently generated across all match wrappers, preserves NULL semantics, and retains the indexable containment conjunct. I found no actionable regressions. |
coderdan
reviewed
Jul 27, 2026
Comment on lines
+366
to
+373
| let (op_hit, fn_hit): (bool, bool) = sqlx::query_as( | ||
| "SELECT ($1::jsonb::public.eql_v3_text_match) @@ ($2::jsonb::public.eql_v3_text_match), | ||
| eql_v3.matches($1::jsonb::public.eql_v3_text_match, $2::jsonb::public.eql_v3_text_match)", | ||
| ) | ||
| .bind(&aardvark) | ||
| .bind(&pq) | ||
| .fetch_one(&pool) | ||
| .await?; |
Contributor
There was a problem hiding this comment.
Not necessary for this pull request, but this pattern would be quite easy to create a property test for, because the properties in plain text are easy to establish. Have something we can do in a follow-up. There might be more pathological examples.
Contributor
Author
There was a problem hiding this comment.
Good idea, will create follow up ticket
freshtonic
deleted the
james/cip-3606-an-empty-bloom-filter-makes-eql_v3matches-vacuously-true-a
branch
July 27, 2026 07:21
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
2 participants
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.
Summary
eql_v3.matches(the@@fuzzy-match operator onpublic.eql_v3_text_match/public.eql_v3_text_search/public.eql_v3_text_search_ore) reduces to bloom array-containmentmatch_term(a) @> match_term(b). An empty needle bloom ({}) is contained by every value, so a search term with no n-gram tokens (a sub-trigram string below the tokeniser floor) matched every row in the table, silently.Fixes CIP-3606 / closes #416.
The fix
The codegen renders the
@@wrapper with an inline empty-needle guard givingLIKE-shaped semantics — an empty needle matches only a value whose own bloom is also empty ('' LIKE ''→ true;'catty' LIKE ''→ false):Why inline, and why non-
STRICTThe issue's proposed fix (a
CASEinmatches(), or abloom_matchhelper) would break GIN index engagement —operator_surface.rs,tasks/test/splinter.sh, and thebare_matches_operator_uses_functional_indextest all requirematches()to inline to a single top-levelmatch_term(col) @> needle. Burying that@>inside aCASE/helper makes it non-indexable.The guard is therefore inline: the top-level
@>conjunct stays an indexable qualifier, soWHERE col @@ $1(bind parameter) or a literal needle still inlines and engages a Bitmap Index Scan, with the guard riding along as a cheap recheck filter. The wrapper is now non-STRICT: PostgreSQL will not inline aSTRICTSQL function whose body carries the guard'sAND/OR, and the body propagatesNULLon aNULLoperand on its own — so droppingSTRICTpreserves the NULL semantics while restoring inlinability.One narrow, documented trade-off: the guard references the needle twice, so a needle supplied as an uncorrelated subquery (
col @@ (SELECT …)) no longer inlines and falls back to a sequence scan. The result is still correct; only the index acceleration is lost. Parameters and literals — the normal query shapes — are unaffected. Verified withEXPLAINacross constant / bind-param / subquery needles.Tests
v3_text_empty_bloom("pq"→ a genuinebf: []from the actual crypto;"aardvark"→ populated), following thev3_text_emptyprecedent — no synthetic blobs.text_smoketest that had encoded the bug as intended behaviour ("every filter must match the empty needle").Verification (all green locally)
codegen (113), text_match (21), text_smoke (8), match_smoke, public-surface (12), operator-equivalents (3), lint (7), splinter (
ERROR=0),types:check(no bindings drift),codegen:parity, build + self-contained.🤖 Generated with Claude Code
Summary by CodeRabbit
eql_v3queries no longer match all rows.LIKE ''semantics (only empty blooms match empty blooms).