fix(lucene): render slice-typed fields as array containment (#224) - #228
Open
Lutherwaves wants to merge 23 commits into
Open
fix(lucene): render slice-typed fields as array containment (#224)#228Lutherwaves wants to merge 23 commits into
Lutherwaves wants to merge 23 commits into
Conversation
The extracted walker's fallback treats its return as final, so the Must/MustNot non-expression-operand recovery chain (serializeColumn, then serializeValue, then Base.RenderParam) that existed pre-refactor needs to live in the driver's fallback closure, not in the dialect- free walker. Also restores NOT-wrapping around a successful chain result for MustNot, which the walker's fallback path does not apply itself. Adds a regression test covering both a raw column operand and a raw scalar operand on Must/MustNot to guard this refactor-fidelity path.
MySQL's default sql_mode lacks ANSI_QUOTES, so double-quoted identifiers were parsed as string literals — every generated MySQL query compared against the constant column name instead of the column.
…ality Closes the core of #224: tags:golang now renders provider-appropriate containment instead of "tags" = ?, which Postgres rejected with 'malformed array literal'.
…ent type Postgres @> requires exactly matching array types; neither narrowing nor widening rescues a mismatch. Map Go int/int64 to bigint[], float64 to double precision[] and float32 to real[], replacing the blanket int[] and numeric[] casts that broke []int64 and []float64 outright.
The @> operator requires exactly matching array types, so a smallint[] column rejects ARRAY[?]::int[]. Give each Go element width the Postgres array type that round-trips it exactly, and correct the comment: GORM never creates array columns, so it cannot justify the mapping.
…alue
Casting an array to text let patterns span element boundaries, so
{alpha,beta} matched '%ha,be%'. Bare field:* stays IS NOT NULL so rows
holding an empty array keep matching.
… test JSON_SEARCH compares under utf8mb4_bin regardless of collation, so the MySQL array-wildcard branch is case-sensitive unlike Postgres ILIKE, SQLite LIKE, and this same provider's own LOWER(...)-wrapped scalar branch. No live MySQL is reachable here to verify a fix, so the SQL is left as-is with the limitation documented in a comment. Also assert the MySQL array-wildcard case no longer contains the old scalar LOWER(...) construct, closing a gap where notSQL was empty.
Turns a runtime database error into a parse error naming the field. Fuzzy is deliberately excluded: it currently executes via ::text similarity, so failing closed there would remove working behaviour.
…renderer tags:(a OR b) took a separate code path with '=' hardcoded, reproducing #224 through documented syntax.
Every prior test compared rendered SQL strings and never ran them, which is structurally why #224 survived 45 test functions.
Also corrects two claims that a Go slice is a JSONB column reachable via dot notation - parser.go explicitly rejects nested access on slices.
`-tags:golang` and `NOT tags:golang` are different go-lucene operators (MustNot vs Not). Only MustNot was dispatched into the drivers' own walker; Not fell through to `driver.Base.RenderParam`, which re-emitted `"tags" = ?` and `"tags" SIMILAR TO ?` — issue #224, still reproducible through that spelling on both the SQL and DynamoDB drivers. Verified against a live Postgres before the fix: NOT tags:golang -> NOT("tags" = ?) ERROR: malformed array literal NOT tags:*go* -> NOT("tags" SIMILAR TO ?) ERROR: operator does not exist Also fixed in this pass, all on array fields: - DynamoDB `tags:(golang OR rust)` handed the whole OR sub-tree to `extractLiteralValue`, binding the stringified expression ("id:golang OR id:rust") as one value. It now expands to one `contains()` per member, mirroring the SQL driver. - DynamoDB accepted `tags:>5` and `tags:[a TO b]`, rendering comparisons that silently match nothing. They now fail closed with an error naming the field, as the SQL driver already did. - A wildcard leaf inside a group (`tags:(golang* OR rust)`) fell through to containment and bound the raw pattern. It now takes the same per-element path as the ungrouped `tags:golang*`. The scalar case had the same shape (binding `hello%` under `=`) and now renders ILIKE. Negation over `field:null` is deliberately kept out of the walker: it still collapses to `IS NOT NULL` for both spellings and both array and scalar fields, matching the base driver rather than emitting `NOT (field IS NULL)`. `TestExecuted_Issue224Repro` re-asserted row counts already covered by `TestExecuted_ArrayContainment`; it now asserts what the issue actually reported — that a nil error from `ParseToSQL` implies executable SQL.
…aviour The array section already claimed `NOT tags:golang` worked; that is now true. Adds the `-tags:golang` and `NOT tags:null` equivalents, the grouped wildcard form, and a DynamoDB paragraph covering contains() expansion and the wildcard/ordering/range rejections.
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR adds array-field detection and containment rendering for SQL and DynamoDB Lucene drivers. It adds recursive logical traversal, provider-specific SQL behavior, validation errors, execution tests, SQLite support, and array query documentation. ChangesArray-aware Lucene rendering
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
Lutherwaves
force-pushed
the
fix/lucene-array-field-containment
branch
from
July 31, 2026 17:02
fbd0b3b to
6cefdf4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
storage/search/lucene/walker.go (1)
96-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAllocate the merged parameter slice explicitly.
append(leftParams, rightParams...)can write into the backing array ofleftParams.leftParamscomes fromrenderNode, which builds its slices withappendand can therefore return a slice with spare capacity. A future caller that keeps a second slice over the same array would then observe mutated parameters. An explicit allocation removes the aliasing risk at no cost.♻️ Proposed refactor
- allParams := append(leftParams, rightParams...) + allParams := make([]any, 0, len(leftParams)+len(rightParams)) + allParams = append(allParams, leftParams...) + allParams = append(allParams, rightParams...)🤖 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 `@storage/search/lucene/walker.go` around lines 96 - 101, Update the parameter merge in the expression-rendering flow around renderNode to allocate a new slice sized for both leftParams and rightParams, then copy both slices into it instead of appending directly to leftParams. Continue returning the merged parameters unchanged for both expr.And and the OR branch.
🤖 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.
Inline comments:
In `@docs/lucene.md`:
- Around line 310-317: The MySQL examples in docs/lucene.md use double-quoted
identifiers instead of the backticks emitted by quoteColumnNameFor("mysql",
...). Update the affected MySQL expressions, including the scalar Equality,
Inclusive range, Exclusive range, Comparison, Wildcard, Has value, and
JSON/array examples, to use backticks for column references while preserving
string literals and SQL behavior.
In `@go.mod`:
- Line 16: Update the CI test job in .github/workflows/ci.yml to set
CGO_ENABLED=1 before running go test -v -cover ./..., ensuring the go-sqlite3
dependency used by sql_executed_test.go can build. Do not change the SQLite
driver.
In `@storage/search/lucene/sql_driver.go`:
- Around line 628-640: Preserve typed containment values across all affected
drivers: in storage/search/lucene/sql_driver.go lines 628-640, update
SQLDriver.renderArrayContains to bind numeric and boolean elements using the
field element type, using JSON_QUOTE only for strings and the appropriate JSON
or numeric casts for MySQL and SQLite; in
storage/search/lucene/dynamodb_driver.go lines 93-97, update containment
serialization to build types.AttributeValue from FieldInfo.Type or explicitly
reject unsupported numeric/boolean arrays. Add numeric and boolean array
coverage to the SQL and DynamoDB array tests.
---
Nitpick comments:
In `@storage/search/lucene/walker.go`:
- Around line 96-101: Update the parameter merge in the expression-rendering
flow around renderNode to allocate a new slice sized for both leftParams and
rightParams, then copy both slices into it instead of appending directly to
leftParams. Continue returning the merged parameters unchanged for both expr.And
and the OR branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e110fba2-8bdd-4e8f-b61e-42035b843c76
📒 Files selected for processing (10)
docs/lucene.mdgo.modstorage/search/lucene/dynamodb_driver.gostorage/search/lucene/dynamodb_driver_test.gostorage/search/lucene/parser.gostorage/search/lucene/parser_test.gostorage/search/lucene/sql_driver.gostorage/search/lucene/sql_driver_test.gostorage/search/lucene/sql_executed_test.gostorage/search/lucene/walker.go
golangci-lint errcheck flagged the unchecked return.
Containment values reach the driver already stringified, so binding them as-is compares a string against a JSON number or boolean and silently matches nothing. Postgres was covered by the ::type[] cast; MySQL, SQLite and DynamoDB were not. - MySQL: CAST(? AS JSON) for numeric/boolean elements. JSON_QUOTE builds a JSON string, which never equals a JSON number. - SQLite: value = json_extract(json(?), '$'). json_each yields numbers as numbers and booleans as 1/0. - DynamoDB: bind N or BOOL attributes instead of stringifying every value into an S attribute. Values are now validated against the element type before rendering, so nums:abc is a parse error rather than a database error — the same 500-instead-of-400 failure this package exists to remove. Two adjacent fixes in the same code path: - Wildcards on non-string arrays are rejected. Postgres errors on ILIKE against an integer and MySQL's JSON_SEARCH only inspects string scalars, so nums:*5* could only ever error or silently match nothing. - MySQL array wildcards now fold case on both sides. JSON_SEARCH compares under utf8mb4_bin regardless of the column's collation, so tags:*Go* did not match ["golang"] — inconsistent with the Postgres and SQLite branches and with MySQL's own scalar wildcard branch. This was left unfixed earlier only because no MySQL instance was available to verify against. Verified by execution on all three providers: SQLite via the executed-SQL tests, MySQL 8.4 and PostgreSQL by running the generated statements with bound parameters. Reported by CodeRabbit on #228. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…fier quoting The MySQL examples used double-quoted identifiers, which MySQL reads as string literals rather than column references; the driver emits backticks. Adds a Non-string elements section covering per-provider containment for numeric and boolean arrays, element-type validation, and why wildcards are rejected on them. Drops the MySQL array wildcard case-sensitivity limitation, now fixed. Reported by CodeRabbit on #228. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The executed-SQL tests use github.com/mattn/go-sqlite3, which needs cgo. Runners default to CGO_ENABLED=1 so this is not a live failure, but pinning it keeps those tests from silently failing to build if that default changes. Reported by CodeRabbit on #228. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@storage/search/lucene/sql_driver.go`:
- Around line 656-668: Include reflect.Uint8 consistently in isNonStringElem and
normalizeArrayElemValue, matching arrayElemCast so named []uint8 slices and
arrays are treated as numeric byte values. Ensure MySQL and SQLite
array-containment bindings receive byte values rather than quoted strings, or
remove the corresponding Uint8 cast branch if Uint8 handling is intentionally
centralized elsewhere.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cdfd61b2-7da9-4972-9054-5630b7d39f0a
📒 Files selected for processing (11)
.github/workflows/ci.ymldocs/lucene.mdgo.modstorage/search/lucene/dynamodb_driver.gostorage/search/lucene/dynamodb_driver_test.gostorage/search/lucene/parser.gostorage/search/lucene/parser_test.gostorage/search/lucene/sql_driver.gostorage/search/lucene/sql_driver_test.gostorage/search/lucene/sql_executed_test.gostorage/search/lucene/walker.go
🚧 Files skipped from review as they are similar to previous changes (7)
- storage/search/lucene/parser_test.go
- storage/search/lucene/parser.go
- go.mod
- docs/lucene.md
- storage/search/lucene/dynamodb_driver.go
- storage/search/lucene/walker.go
- storage/search/lucene/sql_driver_test.go
isArrayField excludes any slice or array of bytes, so a uint8 element cannot reach arrayElemCast. Listing it implied a byte-array code path that does not exist, and left the kind handling looking inconsistent with isNonStringElem and normalizeArrayElemValue, which correctly omit it. Verified: isArrayField reports false for []byte, []uint8, a named []uint8 and [16]byte alike, so no containment binding is affected. Raised by CodeRabbit on #228, whose reading was that named []uint8 slices do pass isArrayField; they do not. Removing the dead branch is the other resolution it offered and the accurate one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ield.go The per-kind knowledge was spread across four switches — arrayElemCast, isNonStringElem, elemBitSize and normalizeArrayElemValue — and had already drifted within this branch: b373e54 removed reflect.Uint8 from arrayElemCast as unreachable, but elemBitSize still carried it. Replaces all four with a single arrayElemSpecs table, one row per kind holding the Postgres cast, the strconv width and whether the kind must not be bound as a string. Adding or removing a kind is now one edit that cannot leave the three facts disagreeing. Moves the dialect-neutral array helpers out of sql_driver.go into a new arrayfield.go. dynamodb_driver.go consumed four of them, so the SQL driver's file had become the de-facto home of shared array semantics with no obvious place for the next one. isArrayField stays in parser.go, where it belongs beside canUseNestedAccess — the predicate that defines its boundary. Also: - Rename the DynamoDB elemType parameters to fieldType. They receive the field's type ([]int), not the element's; only the immediate arrayElemKind unwrap made them work, so the name would have misled the next caller. - Extract columnName, the go-lucene column-shape unwrap that had been copied into serializeColumn, resolveField and arrayFieldName. A fourth upstream shape is now a single edit. - Rename arrayFieldName to resolveArrayField and return a nil type unless the field really is a known array, so the type cannot be read stale. - Make quoteColumnNameFor a method; every call site passed s.provider. - Extract the DynamoDB wildcard rejection, duplicated verbatim twice. - renderArrayWildcard's bare branch returns ref.params instead of nil. Always empty today, but only by an invariant stated nowhere. - Route the last bare-wildcard check through isBareWildcard. - Document that the walker calls fallback with the parent node, not the offending child, and applies no negation wrapping to its result. Adds arrayfield_test.go pinning the cast mapping, the numeric classification, value normalization including width range checks, and the column shapes — the drift guard the four switches lacked. Behaviour-preserving: the cast table was checked against the switch it replaces for all 13 kinds plus nil, non-slice, []byte and pointer-to-slice. Raised by code review of the branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Closes #224.
The bug
A slice-typed model field —
Tags []stringmapped to a Postgrestext[]column — rendered as scalar equality:Postgres rejects that with
malformed array literal. CriticallyParseToSQLreturnederr = nil, so a caller that validated the filter saw it as valid and only discovered the problem as a database error — a 500 where a 400 belonged.Semantics
Containment, matching Lucene/Elasticsearch: a multi-valued field is queried exactly like a single-valued one, so
tags:golangmatches a row whose tags containgolang.tags:golangtags:*go*tags:*COALESCE("tags" @> ARRAY[?], false)EXISTS (SELECT 1 FROM unnest("tags") …ILIKE ?)"tags" IS NOT NULLCOALESCE(JSON_CONTAINS(`tags`, JSON_QUOTE(?)), false)JSON_SEARCH(`tags`, 'one', ?) IS NOT NULL\`tags\` IS NOT NULLEXISTS (SELECT 1 FROM json_each("tags") WHERE value = ?)EXISTS (… json_each … LIKE ?)"tags" IS NOT NULLcontains(tags, ?)contains()on a list is membership, not substring>,<,>=,<=and ranges now return a parse error naming the field, instead of emitting SQL that fails at the database.Why
@>and not= ANYPostgres' own docs lead with
10000 = ANY(pay_by_quarter), but= ANYcannot use a GIN index. Measured on a 200k-row table:docs/lucene.mdnow tells users to create that index, since GORM will not.Second bug fixed along the way
The existing wildcard path cast the whole array to text and substring-matched it, so a pattern could span the separator between two elements. Verified:
{alpha,beta}matched%ha,be%. Per-element matching fixes that.Adjacent fixes (in the way, so fixed here)
quoteColumnNamedouble-quoted identifiers for every provider, but MySQL's defaultsql_modelacksANSI_QUOTES, so"tags"was the string literal'tags'— every MySQL query this package generated compared against a constant instead of a column. Existing MySQL tests only asserted a?appeared, which is why it survived. Now uses backticks.convertWildcardscorrupted exact-match parameters. It ran on every value, sotags:"what?"boundwhat_. Harmless underLIKE, silently wrong under containment. The equality path now bypasses it.NOTkeyword bypassed array handling entirely (found by the final whole-branch review).expr.Notwas never dispatched, soNOT tags:golangfell through to the base driver and reproduced the original bug. Only the-tags:golangspelling worked. Both spellings now work, on all providers.tags:(golang OR rust)) took a separate path with=hardcoded, reproducing the bug through documented syntax. Also fixed the scalar twin, wheretitle:(hello* OR world)boundhello%under=.docs/lucene.mdclaimed in two places that a Go slice is a JSONB column reachable via dot notation.parser.goexplicitly rejects that. Both corrected.Deliberate decisions worth reviewing
tags:*rendersIS NOT NULL, so it still matches rows holding an empty array. Elasticsearch'sexistswould not. Chosen to preserve the existing row set (verified: 3 of 4 fixture rows before and after; the per-element form returns 2). Documented as a divergence.tags:foo~2renderssimilarity(tags::text, ?)today and executes, so rejecting it would remove working behaviour — unlike ranges and ordering operators, which are already broken.::bigint[],::smallint[],::double precision[], …). Postgres@>requires exactly matching array types and rejects both narrowing and widening.Testing
Every pre-existing test in this package compares rendered SQL strings and never executes them — structurally why this bug survived 45 test functions, since
"tags" = ?was exactly the string the code intended to emit.This PR adds
sql_executed_test.go, which drives real filters throughNewParser→ParseToSQLand executes the result against in-memory SQLite, asserting on returned rows. No new dependency (mattn/go-sqlite3was already in the module graph). Stashing the source fix makes those tests fail, so the harness genuinely catches the regression.go test ./...,go vet ./...andgofmtare clean.Known limitations (documented)
JSON_SEARCHcompares underutf8mb4_bin, sotags:*Go*will not match["golang"]there, unlike PostgresILIKEand SQLiteLIKE. No MySQL instance was available to verify a fix, so the behaviour is documented rather than changed blind.gorm:"serializer:json"slices are stored as JSON but detected as native arrays, so Postgres receives@> ARRAY[?]against a JSON column and errors. Use an explicitly JSON-named type for JSON-backed arrays.[]bytefield is a scalar blob, not an array. Slice types whose name containsJSON/JSONBkeep the nested-access path.Follow-ups filed
NewSQLDriversilently ignores a nilFieldInfo.Type, reproducing this bug class with no diagnosticSummary by CodeRabbit
New Features
Bug Fixes
Documentation