Skip to content

fix: order strings byte-for-byte in the VM and search generators - #59

Open
junichi-cstk wants to merge 1 commit into
masterfrom
fix/string-ordering-lexicographic
Open

fix: order strings byte-for-byte in the VM and search generators#59
junichi-cstk wants to merge 1 commit into
masterfrom
fix/string-ordering-lexicographic

Conversation

@junichi-cstk

Copy link
Copy Markdown
Contributor

What was wrong

An audience filtering country < "Argentina" returned an entity with country = "Albania" from Elasticsearch and returned nobody from the VM. Ordering against a string column was implemented three different ways:

Engine country < "Argentina", entity country = "Albania"
esgen {"range":{"plain_country":{"lt":"Argentina"}}} — a keyword field, so a byte-order range → matches
VM operateStrings had no ordering case → ErrorValuealways false, for every entity
blevegen NumericRangeQuery whose int/float64 type switch matched no string bound, leaving both bounds nil → predicate silently dropped

< and >= were both false in the VM. The two halves not being complementary is the tell that it was an error result rather than a comparison — and downstream that reads as a definitive "no", not "unknown".

What changes

All three now compare the way the keyword index already does.

Before After
str < <= > >= ErrorValue strings.Compare
[]str < <= > >= unevaluated (nil, false) true when any element matches, like a term range over a multi-valued field
str BETWEEN a AND b walkTernary had Int/Number/Time only → (nil, false) byte order, exclusive both ends, matching the numeric branches and the gt/lt pair esgen emits
str = != CONTAINS LIKE IN unchanged unchanged
int / number / time columns unchanged unchanged

The generator coercion was not cosmetic

makeRange and coerceScalar (which feeds makeBetween) tried ParseFloat then dateparse.ParseAny on the literal regardless of column type. For a string column that meant:

signedup BETWEEN "2026-01-01" AND "2026-12-31"
  before:  {"gt": 1767225600000}  {"lt": 1798675200000}

ES then compared the keyword term "2026-05-09" against "1778284800000""2" > "1", so every ISO date in the index sorted greater. Both now keep the literal verbatim for StringType/StringsType/MapStringType; other types are untouched, so visitct < "10" still coerces to int64(10).

Verification

  • Every new test was confirmed failing against the pre-fix code with go test -overlay, which never touches the worktree. Representative pre-fix output: esgen produced LT: 1.7782848e+12 where "2026-05-09" was expected; the VM cases reported should have non-nil result but was nil. The VM's table test fatals on first mismatch, so the []string and BETWEEN cases were isolated under a second overlay to confirm each fails on its own rather than riding along.
  • The bleve tests search a real in-memory index with a keyword field mapping rather than asserting query shape. Pre-fix they returned 0 hits for every ordering and BETWEEN case; now 1/2/1/2 and 1/2/1/0. (An earlier revision of the fixture used the default analyzer, which lowercases terms and made the query look broken when it wasn't — hence the explicit keyword mapping.)
  • go build ./..., go vet ./... and go test ./... all clean on this base.
  • Downstream: lio's segment compiler, qlindex, segutil and segmembership suites pass against this branch via a local replace. The paired lio change is https://github.com/lytics/lio/pull/39220, which stays red until this merges and lio's pin moves — that is deliberate, since lio's compiled evaluator bypasses operateStrings and the two semantics must not diverge per-account.

Left alone deliberately

  • blevegen.makeBetween's numeric path uses inclusive bounds while esgen and the VM are exclusive. Pre-existing, out of scope, and bleve is not a production query path in lio.
  • vm/vm_test.go's fixture map was already not gofmt-clean on master; reformatting it would bury this diff in noise.
  • One existing VM case changed meaning rather than being deleted: user_id > "abc" asserted evalError and now asserts false, which is the behavior this PR is introducing.

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) <noreply@anthropic.com>
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.

1 participant