From fc207426db98a3f0a6abd852769b2267997b7d19 Mon Sep 17 00:00:00 2001 From: Junichi Furukawa Date: Thu, 20 Aug 2026 14:46:38 -0700 Subject: [PATCH] fix: order strings byte-for-byte in the VM and search generators An audience filtering `country < "Argentina"` matched an entity with country "Albania" in Elasticsearch and matched nobody in the VM. Three engines implemented ordering against a string column three ways: ES {"range":{"plain_country":{"lt":"Argentina"}}} on a keyword field, so a byte-order range -> matches VM operateStrings had no ordering case, returned an ErrorValue -> always false, for every entity bleve NumericRangeQuery whose int/float type switch matched no string bound, leaving both bounds nil -> predicate dropped `<` and `>=` were both false in the VM, so the two halves were not complementary -- the tell that it was an error result rather than a comparison. Make all three compare the way the keyword index already does: - operateStrings handles <, <=, > and >=; the StringsValue case gets the same operators with any-element semantics, matching how an index evaluates a term range over a multi-valued field. - walkTernary gains a StringValue BETWEEN branch, exclusive on both ends like the numeric branches and like the gt/lt pair esgen emits. - makeRange and coerceScalar (which feeds makeBetween) stop coercing the literal for string-typed columns. This was not cosmetic: an ISO bound became epoch millis, so ES compared the keyword "2026-05-09" against "1778284800000" and every term sorted greater. - blevegen emits term ranges for string columns in both makeRange and makeBetween. Every new test was confirmed failing against the pre-fix code with `go test -overlay`. The bleve tests search a real in-memory index with a keyword mapping rather than asserting query shape; pre-fix they returned 0 hits for every ordering and BETWEEN case. Full suite green. Two pre-existing inconsistencies left alone: blevegen's numeric makeBetween uses inclusive bounds while esgen and the VM are exclusive, and vm/vm_test.go's fixture map was already not gofmt-clean. Co-Authored-By: Claude Opus 5 (1M context) --- generators/blevegen/blevegenerator_test.go | 107 +++++++++++++++++++++ generators/blevegen/bridgeutil.go | 69 +++++++++++++ generators/esgen/bridgeutil.go | 19 +++- generators/esgen/esgenerator_test.go | 94 ++++++++++++++++++ vm/vm.go | 47 +++++++++ vm/vm_test.go | 17 +++- 6 files changed, 347 insertions(+), 6 deletions(-) diff --git a/generators/blevegen/blevegenerator_test.go b/generators/blevegen/blevegenerator_test.go index d4ca785..61e00ec 100644 --- a/generators/blevegen/blevegenerator_test.go +++ b/generators/blevegen/blevegenerator_test.go @@ -314,3 +314,110 @@ func (s schema) ColumnInfo(f string) (*gentypes.FieldType, bool) { TypeName: c.String(), }, true } + +// TestStringOrderingTermRange covers ordering operators against a string-typed +// column. A NumericRangeQuery leaves both bounds nil for a string literal, +// which silently drops the predicate and matches every document; the term +// range has to compare byte-for-byte the way vm.operateStrings does. +func TestStringOrderingTermRange(t *testing.T) { + // country must be untokenized; an analyzed field lowercases the terms and + // then compares them against a literal that was not lowercased. + countryMapping := bleve.NewKeywordFieldMapping() + countryMapping.Name = "country" + docMapping := bleve.NewDocumentMapping() + docMapping.AddFieldMappingsAt("country", countryMapping) + indexMapping := bleve.NewIndexMapping() + indexMapping.DefaultMapping = docMapping + + idx, err := bleve.NewMemOnly(indexMapping) + require.NoError(t, err) + t.Cleanup(func() { idx.Close() }) + + for id, country := range map[string]string{ + "d1": "Albania", + "d2": "Argentina", + "d3": "Zimbabwe", + } { + require.NoError(t, idx.Index(id, map[string]any{"country": country})) + } + + countrySchema := schema{cols: map[string]value.ValueType{"country": value.StringType}} + + tests := []struct { + filterQL string + want int + }{ + {`FILTER country < "Argentina"`, 1}, // Albania + {`FILTER country <= "Argentina"`, 2}, // Albania, Argentina + {`FILTER country > "Argentina"`, 1}, // Zimbabwe + {`FILTER country >= "Argentina"`, 2}, // Argentina, Zimbabwe + {`FILTER country < "Albania"`, 0}, + } + + for _, tc := range tests { + t.Run(tc.filterQL, func(t *testing.T) { + filter, err := rel.ParseFilterQL(tc.filterQL) + require.NoError(t, err) + + payload, err := NewGenerator(time.Now(), nil, countrySchema).WalkExpr(filter.Filter) + require.NoError(t, err) + + q, ok := payload.Filter.(query.Query) + require.True(t, ok) + res, err := idx.Search(bleve.NewSearchRequest(q)) + require.NoError(t, err) + assert.Equal(t, tc.want, int(res.Total)) + }) + } +} + +// TestStringBetweenTermRange covers BETWEEN against a string-typed column, +// which hit the same nil-bounds NumericRangeQuery defect as makeRange. +func TestStringBetweenTermRange(t *testing.T) { + countryMapping := bleve.NewKeywordFieldMapping() + countryMapping.Name = "country" + docMapping := bleve.NewDocumentMapping() + docMapping.AddFieldMappingsAt("country", countryMapping) + indexMapping := bleve.NewIndexMapping() + indexMapping.DefaultMapping = docMapping + + idx, err := bleve.NewMemOnly(indexMapping) + require.NoError(t, err) + t.Cleanup(func() { idx.Close() }) + + for id, country := range map[string]string{ + "d1": "Albania", + "d2": "Argentina", + "d3": "Zimbabwe", + } { + require.NoError(t, idx.Index(id, map[string]any{"country": country})) + } + + countrySchema := schema{cols: map[string]value.ValueType{"country": value.StringType}} + + tests := []struct { + filterQL string + want int + }{ + {`FILTER country BETWEEN "AAA" AND "Argentina"`, 1}, // Albania; upper is exclusive + {`FILTER country BETWEEN "AAA" AND "Zimbabwe"`, 2}, // Albania, Argentina + {`FILTER country BETWEEN "Albania" AND "Zimbabwe"`, 1}, // lower is exclusive + {`FILTER country BETWEEN "AAA" AND "AAB"`, 0}, + } + + for _, tc := range tests { + t.Run(tc.filterQL, func(t *testing.T) { + filter, err := rel.ParseFilterQL(tc.filterQL) + require.NoError(t, err) + + payload, err := NewGenerator(time.Now(), nil, countrySchema).WalkExpr(filter.Filter) + require.NoError(t, err) + + q, ok := payload.Filter.(query.Query) + require.True(t, ok) + res, err := idx.Search(bleve.NewSearchRequest(q)) + require.NoError(t, err) + assert.Equal(t, tc.want, int(res.Total)) + }) + } +} diff --git a/generators/blevegen/bridgeutil.go b/generators/blevegen/bridgeutil.go index 2b790be..1bdd14a 100644 --- a/generators/blevegen/bridgeutil.go +++ b/generators/blevegen/bridgeutil.go @@ -41,6 +41,9 @@ func makeRange(lhs *gentypes.FieldType, op lex.TokenType, rhs expr.Node) (query. return nil, fmt.Errorf("Could not convert %T %v to float", rhsval, rhsval) } rhsval = fv + case value.StringType, value.StringsType, value.MapStringType: + // Untokenized string fields range byte-for-byte, so the literal has to + // survive as written; see makeTermRange below. default: if rhsstr, ok := rhsval.(string); ok { if rhsf, err := strconv.ParseFloat(rhsstr, 64); err == nil { @@ -52,6 +55,13 @@ func makeRange(lhs *gentypes.FieldType, op lex.TokenType, rhs expr.Node) (query. fieldName := lhs.Field + switch lhs.Type { + case value.StringType, value.StringsType, value.MapStringType: + // A NumericRangeQuery leaves both bounds nil for a string literal, which + // silently drops the predicate instead of comparing anything. + return makeTermRange(fieldName, op, rhsval) + } + // Create a range query rangeQuery := query.NewNumericRangeQuery(nil, nil) rangeQuery.SetField(fieldName) @@ -129,6 +139,38 @@ func makeRange(lhs *gentypes.FieldType, op lex.TokenType, rhs expr.Node) (query. return rangeQuery, nil } +// makeTermRange returns a byte-order term range for untokenized string fields, +// matching vm.operateStrings. +func makeTermRange(fieldName string, op lex.TokenType, rhsval any) (query.Query, error) { + term, ok := rhsval.(string) + if !ok { + return nil, fmt.Errorf("qlindex: string field range needs a string, got %T", rhsval) + } + + if term == "" { + // bleve reads an empty bound as "unbounded", so it cannot express a + // range against the empty string. + return nil, fmt.Errorf("qlindex: cannot range %s against an empty string", op) + } + + t, f := true, false + var rangeQuery *query.TermRangeQuery + switch op { + case lex.TokenGE: + rangeQuery = query.NewTermRangeInclusiveQuery(term, "", &t, &f) + case lex.TokenGT: + rangeQuery = query.NewTermRangeInclusiveQuery(term, "", &f, &f) + case lex.TokenLE: + rangeQuery = query.NewTermRangeInclusiveQuery("", term, &f, &t) + case lex.TokenLT: + rangeQuery = query.NewTermRangeInclusiveQuery("", term, &f, &f) + default: + return nil, fmt.Errorf("qlindex: unsupported range operator %s", op) + } + rangeQuery.SetField(fieldName) + return rangeQuery, nil +} + // makeDateRangeQuery creates a date range query for time-based fields func makeDateRangeQuery(fieldName string, op lex.TokenType, rhsval any) (query.Query, error) { var timeVal time.Time @@ -198,6 +240,13 @@ func makeBetween(lhs *gentypes.FieldType, lower, upper any) (query.Query, error) return makeDateBetweenQuery(fieldName, lower, upper) } + switch lhs.Type { + case value.StringType, value.StringsType, value.MapStringType: + // Same defect as makeRange had: string bounds match no numeric case + // below, leaving both bounds nil and dropping the predicate. + return makeTermBetween(fieldName, lower, upper) + } + // Create a numeric range query rangeQuery := query.NewNumericRangeQuery(nil, nil) rangeQuery.SetField(fieldName) @@ -235,6 +284,26 @@ func makeBetween(lhs *gentypes.FieldType, lower, upper any) (query.Query, error) return rangeQuery, nil } +// makeTermBetween returns a byte-order term range for an untokenized string +// field. Exclusive on both ends, matching vm.walkTernary and the gt/lt pair +// esgen emits. +func makeTermBetween(fieldName string, lower, upper any) (query.Query, error) { + lo, loOK := lower.(string) + hi, hiOK := upper.(string) + if !loOK || !hiOK { + return nil, fmt.Errorf("qlindex: string field BETWEEN needs string bounds, got %T and %T", lower, upper) + } + if lo == "" || hi == "" { + // bleve reads an empty bound as "unbounded". + return nil, fmt.Errorf("qlindex: cannot range BETWEEN against an empty string") + } + + f := false + rangeQuery := query.NewTermRangeInclusiveQuery(lo, hi, &f, &f) + rangeQuery.SetField(fieldName) + return rangeQuery, nil +} + // makeDateBetweenQuery creates a date range query for BETWEEN operations on time fields func makeDateBetweenQuery(fieldName string, lower, upper any) (query.Query, error) { // Create a date range query diff --git a/generators/esgen/bridgeutil.go b/generators/esgen/bridgeutil.go index a509ddf..5e93f0b 100644 --- a/generators/esgen/bridgeutil.go +++ b/generators/esgen/bridgeutil.go @@ -45,6 +45,10 @@ func makeRange(lhs *gentypes.FieldType, op lex.TokenType, rhs expr.Node) (any, e return nil, fmt.Errorf("Could not convert %T %v to float", rhsval, rhsval) } rhsval = fv + case value.StringType, value.StringsType, value.MapStringType: + // Keyword fields range byte-for-byte, so the literal has to reach ES as + // written. Coercing it would compare an epoch-millis number against a + // country name, and would not match vm.operateStrings either. default: if rhsstr, ok := rhsval.(string); ok { if rhsf, err := strconv.ParseFloat(rhsstr, 64); err == nil { @@ -299,11 +303,12 @@ func makeBetween(lhs *gentypes.FieldType, lower, upper any) (any, error) { // produce consistent queries. // // For IntType fields the value is converted to int64. For NumberType fields it -// is converted to float64. For all other types (including TimeType) string -// values are first tried as a float (epoch-millis strings like "1778310000000") -// and then as an ISO date string (e.g. "2026-05-09"), which is converted to -// epoch milliseconds. Values that cannot be coerced are returned unchanged so -// that Elasticsearch can attempt its own parsing. +// is converted to float64. String-typed fields keep the literal verbatim. For +// all other types (including TimeType) string values are first tried as a float +// (epoch-millis strings like "1778310000000") and then as an ISO date string +// (e.g. "2026-05-09"), which is converted to epoch milliseconds. Values that +// cannot be coerced are returned unchanged so that Elasticsearch can attempt +// its own parsing. func coerceScalar(lhs *gentypes.FieldType, val any) any { rhv := value.NewValue(val) switch lhs.Type { @@ -315,6 +320,10 @@ func coerceScalar(lhs *gentypes.FieldType, val any) any { if fv, ok := value.ValueToFloat64(rhv); ok { return fv } + case value.StringType, value.StringsType, value.MapStringType: + // Same reason as makeRange: keyword fields range byte-for-byte, so the + // bound has to reach ES as written. + return val default: s, ok := val.(string) if !ok { diff --git a/generators/esgen/esgenerator_test.go b/generators/esgen/esgenerator_test.go index 971bafd..bfb3bc9 100644 --- a/generators/esgen/esgenerator_test.go +++ b/generators/esgen/esgenerator_test.go @@ -304,3 +304,97 @@ func assertJSONEqual(t *testing.T, want string, got any) { require.NoError(t, json.Unmarshal([]byte(want), &wantNorm)) assert.Equal(t, wantNorm, gotNorm, "generated ES filter mismatch\nwant: %s\ngot: %s", want, string(gotBytes)) } + +// TestStringOrderingRange covers ordering operators against a string-typed +// column. The literal must reach Elasticsearch as written: a keyword field +// ranges byte-for-byte, so coercing a numeric- or date-looking literal to a +// number would compare against a different value than vm.operateStrings does. +func TestStringOrderingRange(t *testing.T) { + s := schema{cols: map[string]value.ValueType{ + "country": value.StringType, + "score": value.StringType, + "signedup": value.StringType, + "visitct": value.IntType, + }} + g := NewGenerator(time.Now(), nil, s) + + tests := []struct { + name string + filterQL string + field string + want RangeQry + }{ + {"LtWord", `FILTER country < "Argentina"`, "country", RangeQry{LT: "Argentina"}}, + {"GtWord", `FILTER country > "Argentina"`, "country", RangeQry{GT: "Argentina"}}, + {"LeWord", `FILTER country <= "Argentina"`, "country", RangeQry{LTE: "Argentina"}}, + {"GeWord", `FILTER country >= "Argentina"`, "country", RangeQry{GTE: "Argentina"}}, + // "0.2" must stay a string; as a float it round-trips to a different + // term than the one the VM compares against. + {"NumericLooking", `FILTER score > "0.20"`, "score", RangeQry{GT: "0.20"}}, + // Coerced to epoch millis this became a number whose leading digit made + // every ISO date in the index compare greater. + {"DateLooking", `FILTER signedup < "2026-05-09"`, "signedup", RangeQry{LT: "2026-05-09"}}, + // Non-string columns keep their coercion. + {"IntColumnStillCoerces", `FILTER visitct < "10"`, "visitct", RangeQry{LT: int64(10)}}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + fs, err := rel.ParseFilterQL(tc.filterQL) + require.NoError(t, err) + p, err := g.WalkExpr(fs.Filter) + require.NoError(t, err) + r, ok := p.Filter.(*RangeFilter) + require.True(t, ok, "expected a range filter, got %T", p.Filter) + assert.Equal(t, tc.want, r.Range[tc.field]) + }) + } +} + +// TestStringBetweenRange covers BETWEEN against a string-typed column. +// coerceScalar coerced date- and numeric-looking bounds the same way makeRange +// did, so an ISO bound reached ES as epoch millis and every keyword term +// compared greater. +func TestStringBetweenRange(t *testing.T) { + s := schema{cols: map[string]value.ValueType{ + "country": value.StringType, + "signedup": value.StringType, + "visitct": value.IntType, + }} + g := NewGenerator(time.Now(), nil, s) + + tests := []struct { + name string + filterQL string + field string + wantLower any + wantUpper any + }{ + {"Words", `FILTER country BETWEEN "AAA" AND "Argentina"`, "country", "AAA", "Argentina"}, + {"DateLooking", `FILTER signedup BETWEEN "2026-01-01" AND "2026-12-31"`, "signedup", "2026-01-01", "2026-12-31"}, + {"IntColumnStillCoerces", `FILTER visitct BETWEEN "1" AND "10"`, "visitct", int64(1), int64(10)}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + fs, err := rel.ParseFilterQL(tc.filterQL) + require.NoError(t, err) + p, err := g.WalkExpr(fs.Filter) + require.NoError(t, err) + + b, ok := p.Filter.(*boolean) + require.True(t, ok, "expected a bool filter, got %T", p.Filter) + m, ok := b.Bool.(must) + require.True(t, ok, "expected a must clause, got %T", b.Bool) + require.Len(t, m.Filters, 2) + + lower, ok := m.Filters[0].(*RangeFilter) + require.True(t, ok) + upper, ok := m.Filters[1].(*RangeFilter) + require.True(t, ok) + + assert.Equal(t, tc.wantLower, lower.Range[tc.field].GT) + assert.Equal(t, tc.wantUpper, upper.Range[tc.field].LT) + }) + } +} diff --git a/vm/vm.go b/vm/vm.go index 9f3bcb0..659a4e7 100644 --- a/vm/vm.go +++ b/vm/vm.go @@ -728,6 +728,19 @@ func evalBinary(ctx expr.EvalContext, includer expr.Includer, node *expr.BinaryN } return value.BoolValueFalse, true } + case lex.TokenLT, lex.TokenLE, lex.TokenGT, lex.TokenGE: + // Any-element semantics, matching how a search index evaluates a + // term range against a multi-valued field. + bv, ok := br.(value.StringValue) + if !ok { + return nil, false + } + for _, val := range at.Val() { + if compareOrdered(strings.Compare(val, bv.Val()), node.Operator.T) { + return value.BoolValueTrue, true + } + } + return value.BoolValueFalse, true case lex.TokenLogicOr, lex.TokenOr, lex.TokenEqualEqual, lex.TokenEqual, lex.TokenLogicAnd, lex.TokenAnd: return value.NewBoolValue(false), true @@ -941,6 +954,21 @@ func walkTernary(ctx expr.EvalContext, includer expr.Includer, node *expr.TriNod } return value.NewBoolValue(false), true + case value.StringValue: + av := at.Val() + // Exclusive on both ends, matching the numeric branches above and + // the gt/lt pair the search generators emit for BETWEEN. + if strings.Compare(av, b.ToString()) > 0 && strings.Compare(av, c.ToString()) < 0 { + if node.Negated() { + return value.NewBoolValue(false), true + } + return value.NewBoolValue(true), true + } + if node.Negated() { + return value.NewBoolValue(true), true + } + return value.NewBoolValue(false), true + default: u.Warnf("between not implemented for type %s %#v", a.Type().String(), node) } @@ -1099,10 +1127,29 @@ func operateStrings(op lex.Token, av, bv value.StringValue) value.Value { return value.BoolValueTrue } return value.BoolValueFalse + case lex.TokenLT, lex.TokenLE, lex.TokenGT, lex.TokenGE: + // Byte-order comparison, matching a SQL collation-free ORDER BY and the + // term-range semantics of a keyword/untokenized index field. + return value.NewBoolValue(compareOrdered(strings.Compare(a, b), op.T)) } return value.NewErrorValuef("unsupported operator for strings: %s", op.T) } +// compareOrdered maps a -1/0/1 comparison result onto an ordering operator. +func compareOrdered(cmp int, op lex.TokenType) bool { + switch op { + case lex.TokenLT: + return cmp < 0 + case lex.TokenLE: + return cmp <= 0 + case lex.TokenGT: + return cmp > 0 + case lex.TokenGE: + return cmp >= 0 + } + return false +} + func operateTime(op lex.TokenType, lht, rht time.Time) (value.BoolValue, bool) { switch op { case lex.TokenEqual, lex.TokenEqualEqual: diff --git a/vm/vm_test.go b/vm/vm_test.go index e0c22ea..34fd15f 100644 --- a/vm/vm_test.go +++ b/vm/vm_test.go @@ -136,6 +136,11 @@ var ( vmt(`10 BETWEEN int5 AND 50`, true, noError), vmtall(`10 BETWEEN 20 AND true`, nil, parseOk, evalError), vmt(`created BETWEEN "12/18/2015" AND "12/18/2050"`, true, noError), + // Byte-order BETWEEN, exclusive on both ends. user_id is "abc". + vmt(`user_id BETWEEN "aaa" AND "abd"`, true, noError), + vmt(`user_id BETWEEN "abc" AND "abd"`, false, noError), + vmt(`user_id BETWEEN "aaa" AND "abc"`, false, noError), + vmt(`user_id BETWEEN "b" AND "c"`, false, noError), vmt(`created BETWEEN "now-50w" AND "12/18/2050"`, true, noError), // In: Multi Arg Tests @@ -169,7 +174,17 @@ var ( vmt(`user_id != "abcd"`, true, noError), vmt(`user_id == "abcd"`, false, noError), vmt(`user_id != "abc"`, false, noError), - vmtall(`user_id > "abc"`, nil, parseOk, evalError), + // Byte-order comparison; user_id is "abc". + vmt(`user_id > "abc"`, false, noError), + vmt(`user_id >= "abc"`, true, noError), + vmt(`user_id < "abd"`, true, noError), + vmt(`user_id <= "abb"`, false, noError), + vmt(`user_id > "ABC"`, true, noError), + vmt(`empty_str < "a"`, true, noError), + // Multi-valued field: true when any element satisfies. urls is [abc 123]. + vmt(`urls > "abb"`, true, noError), + vmt(`urls < "2"`, true, noError), + vmt(`urls < "0"`, false, noError), vmt(`user_id LIKE "*bc"`, true, noError), vmt(`user_id LIKE "\*bc"`, false, noError), vmt(`user_id != NULL`, true, noError),