fix(translator): % Split without explicitly provided identity context translate to FALSE#16
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the SQL flag engine translator to allow percentage splits to be translatable even when the evaluation context does not carry an identity. It removes the checks that previously collapsed these conditions to FALSE, allowing the engine to hash the per-row identity key or identifier instead. Unit tests have been updated to verify this new behavior. I have no feedback to provide as there are no review comments.
khvn26
force-pushed
the
fix/percentage-split-row-translation
branch
from
0e23b2e to
0b5fd4f
Compare
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 0c7ec9a |
khvn26
force-pushed
the
fix/percentage-split-row-translation
branch
from
0b5fd4f to
85f2412
Compare
…t identity A PERCENTAGE_SPLIT keyed on the identity (implicit `$.identity.key`, `$.identity.key`, or `$.identity.identifier`) compiled to FALSE when the evaluation context carried no identity. The translated predicate runs per-row over IDENTITIES, where the identity-key and identifier columns are always present, so it should hash the row column regardless of whether the row-oriented context supplies an identity. The guard made row-oriented callers (segment-membership counts/members) see every percentage-split segment as empty. Drop the eval-context identity guard for those three cases and emit the per-row hash. Trait-keyed splits are unchanged. This intentionally diverges from flag_engine's single-eval "no identity -> False" for the identity-less context, which is correct for the row-oriented SQL engine where every row is an identity; the one engine-test-data case that exercises it (test_percentage_split__no_identity_key__should_match) is added to the ClickHouse harness's _XFAIL_CASE_NAMES. The translator unit tests assert the full emitted SQL. beep boop Claude-Session: https://claude.ai/code/session_01EgZ5iHpDASZzCapiHRxHLB
khvn26
force-pushed
the
fix/percentage-split-row-translation
branch
from
85f2412 to
6de406d
Compare
emyller
left a comment
emyller
left a comment
There was a problem hiding this comment.
Overall I think neither the PR description, nor the patch within, make it clear what the real goal is, e.g. helping towards this.
we drops the eval-context identity guard for those three cases and emits the per-row hash
Please improve perception of why in better comments and tests, or let me know if you'd rather unblock other work first and need an approval.
khvn26
changed the title
FALSEFALSE
emyller
left a comment
emyller
left a comment
There was a problem hiding this comment.
Tests could still look less over-reaching, but not blocking.
3 participants
Contributes to Flagsmith/flagsmith#7911.
In this PR, we drops the eval-context identity guard for those three cases and emits the per-row hash. Trait-keyed splits are unchanged.
It intentionally diverges from
flag_engine's single-eval "no identity → False" verdict for the identity-less context. That verdict is right for a single in-memory evaluation; it isn't for the SQL engine, whose whole job is to evaluate over identity rows where the key always exists.TranslateContext's docstring already states the identity field is ignored because values come from each row — this makes the percentage-split path honour that.