fix(vm): treat a present-but-empty collection as not EXISTS - #60
Open
sumankarki wants to merge 1 commit into
Open
fix(vm): treat a present-but-empty collection as not EXISTS#60sumankarki wants to merge 1 commit into
sumankarki wants to merge 1 commit into
Conversation
EXISTS on a present field returned true for any value that was not nil/NilValue, so an empty list or map (e.g. an empty StringsValue) evaluated as EXISTS=true, NOT EXISTS=false. Elasticsearch does not index empty arrays and reports them as not existing, so the VM and the esgen path disagreed on `NOT EXISTS <collection>` for a present empty value -- a segment could match in an ES-backed scan yet be dropped by the VM re-eval. Scope EXISTS to return false for empty collections (lists and maps), matching ES. Scalars, including an empty string, are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Fixes the EXISTS/empty-collection divergence behind lytics/lio#39226 (LYT-1022). Verified on a real 108k-member audience. |
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.
What
EXISTS <field>in the VM returnedtruefor any present value that was notnil/NilValue. A present-but-empty collection (an emptyStringsValue,SliceValue, or anyMap*Value) is not nil, so it evaluated asEXISTS = true/NOT EXISTS = false.Elasticsearch does not index empty arrays, so the esgen path treats a present empty array as not existing (
NOT EXISTS = true). The two engines therefore disagreed onNOT EXISTS <collection>whenever a field was persisted as a present empty value.This scopes the VM's
EXISTSto returnfalsefor empty collections, matching ES.[]/{}NOT EXISTSis the negation, so it flips correspondingly for the empty case.Why it matters
In a downstream CDP, an ES-backed segment scan selected members where
NOT EXISTS <field>was true (field stored as empty[]), but the VM re-eval on the same entities returned "not a member" and dropped them — so a whole audience matched in the UI/backfill yet produced nothing on export. Verified against a real 108k-member audience: 100% had the field present-but-empty.Scope / blast radius
StringsValue,SliceValue,ByteSliceValue, and theMap*Valuefamily). Scalars are untouched — a present empty string (EXISTS "") is stilltrue, as pinned by the existingempty_strtest.EXISTS/NOT EXISTSon a collection field that can be present-but-empty. This is the intended correction (align VM with ES).Tests
vm/vm_test.go:EXISTS empty_strs→ false,NOT EXISTS empty_strs→ true,EXISTS empty_map→ false, plus populated (urls,hits) → true and scalarempty_str→ true (unchanged).go test ./...passes; no existing test relied on the old behavior.-overlay: reverting the logic makesEXISTS empty_strsfail withtrue != false, confirming the new tests actually exercise the change.