Skip to content

fix(translator): % Split without explicitly provided identity context translate to FALSE #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
khvn26 merged 5 commits into from
Jun 30, 2026
Merged

fix(translator): % Split without explicitly provided identity context translate to FALSE #16

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6de406d
fix(translator): Hash identity-key percentage splits without a contex…
khvn26 Jun 29, 2026
e5011a3
deslop identity key comment
khvn26 Jun 29, 2026
e7e6af4
Deslop XFAIL exclusion comment
khvn26 Jun 29, 2026
4196d63
fix typo
khvn26 Jun 30, 2026
0c7ec9a
test: assert % split does not fold to FALSE instead of pinning full M…
khvn26 Jun 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions src/flagsmith_sql_flag_engine/translator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,19 +435,14 @@ def translate_condition(cond: SegmentCondition, ctx: TranslateContext) -> str |
identity: dict[str, object] = ctx.evaluation_context.get("identity") or {} # type: ignore[assignment]
kind = classification.kind
if not prop:
# Implicit `$.identity.key` — engine returns False when no
# identity, or when the identity lacks `key`. The engine
# never synthesises one from env+identifier.
if not identity.get("key"):
return "FALSE"
# Implicit `$.identity.key`. The key is always present in the store,
# so the split is translatable whether or not the evaluation context
# carries an identity. This intentionally diverges from the engine's
# "no identity → False" verdict.
Comment thread
emyller marked this conversation as resolved.
Outdated
value_expr = ctx.dialect.cast_string(ctx.identity_key_expr)
elif kind == "key":
if not identity.get("key"):
return "FALSE"
value_expr = ctx.dialect.cast_string(ctx.jsonpath_expr("$.identity.key"))
elif kind == "identifier":
if not identity.get("identifier"):
return "FALSE"
value_expr = ctx.dialect.cast_string(ctx.jsonpath_expr("$.identity.identifier"))
elif kind == "identity_object":
# PERCENTAGE_SPLIT on `$.identity` — the whole dict. Engine
Expand Down
6 changes: 6 additions & 0 deletions tests/harnesses/clickhouse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@
# condition, so we accept the divergence on this niche shape
# (`$.`-prefixed trait names) and let callers fall back to the engine.
"test_jsonpath_like_trait__existing_jsonpath__should_match_trait",
# Engine returns False for a PERCENTAGE_SPLIT when the eval context has
# no identity key. The SQL engine intentionally diverges: it runs over
# IDENTITIES rows where the identity key always exists, so it hashes the
# row's key rather than folding to FALSE — the behaviour row-oriented
# callers (segment-membership counts/members) need.
"test_percentage_split__no_identity_key__should_match",
}


Expand Down
62 changes: 47 additions & 15 deletions tests/test_translator_unit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,9 +542,10 @@ def _ctx_identity_without(field: str) -> TranslateContext:
)


def test_translate_segment__percentage_split_implicit_key_no_identity__compiles_to_false() -> None:
# Given a PERCENTAGE_SPLIT with no property (implicit `$.identity.key`) and an eval
# context with no identity
def test_translate_segment__percentage_split_implicit_key_no_identity__inlines_md5() -> None:
# Given a PERCENTAGE_SPLIT with no property (implicit `$.identity.key`) and an
# eval context with no identity — the row-oriented case (e.g. segment-membership
# counting over the whole IDENTITIES table)
seg: SegmentContext = {
"key": "ps2",
"name": "s",
Expand All @@ -556,12 +557,25 @@ def test_translate_segment__percentage_split_implicit_key_no_identity__compiles_
],
}

# When / Then the predicate collapses to FALSE — engine returns False without identity
assert translate_segment(seg, _ctx_no_identity()) == "((FALSE))"
# When translated
sql = translate_segment(seg, _ctx_no_identity())

# Then it hashes the per-row identity-key column rather than folding to
# FALSE — the row supplies the subject, so a context identity is not needed
Comment thread
emyller marked this conversation as resolved.
Outdated
assert sql == (
"(((modulo(reinterpretAsUInt32(reverse(substring(MD5('ps2' || ',' ||"
" (toString(i.identity_key))), 1, 4))) * 7291 +"
" reinterpretAsUInt32(reverse(substring(MD5('ps2' || ',' ||"
" (toString(i.identity_key))), 5, 4))) * 1897 +"
" reinterpretAsUInt32(reverse(substring(MD5('ps2' || ',' ||"
" (toString(i.identity_key))), 9, 4))) * 6835 +"
" reinterpretAsUInt32(reverse(substring(MD5('ps2' || ',' ||"
" (toString(i.identity_key))), 13, 4))), 9999) / 9998.0 * 100.0 <= 50.0)))"
)
Comment thread
khvn26 marked this conversation as resolved.
Outdated


def test_translate_segment__percentage_split_identity_key_missing__compiles_to_false() -> None:
# Given the eval context's identity has no `key`
def test_translate_segment__percentage_split_key_no_context_identity__inlines_md5() -> None:
# Given a PERCENTAGE_SPLIT on `$.identity.key` and a context identity without `key`
seg: SegmentContext = {
"key": "ps3",
"name": "s",
Expand All @@ -575,14 +589,23 @@ def test_translate_segment__percentage_split_identity_key_missing__compiles_to_f
],
}

# When / Then the predicate collapses to FALSE
assert translate_segment(seg, _ctx_identity_without("key")) == "((FALSE))"
# When / Then it still hashes the per-row identity-key column
sql = translate_segment(seg, _ctx_identity_without("key"))
assert sql == (
"(((modulo(reinterpretAsUInt32(reverse(substring(MD5('ps3' || ',' ||"
" (toString(i.identity_key))), 1, 4))) * 7291 +"
" reinterpretAsUInt32(reverse(substring(MD5('ps3' || ',' ||"
" (toString(i.identity_key))), 5, 4))) * 1897 +"
" reinterpretAsUInt32(reverse(substring(MD5('ps3' || ',' ||"
" (toString(i.identity_key))), 9, 4))) * 6835 +"
" reinterpretAsUInt32(reverse(substring(MD5('ps3' || ',' ||"
" (toString(i.identity_key))), 13, 4))), 9999) / 9998.0 * 100.0 <= 50.0)))"
)


def test_translate_segment__percentage_split_identity_identifier_missing__compiles_to_false() -> (
None
):
# Given the eval context's identity has no `identifier`
def test_translate_segment__percentage_split_identifier_no_context_identity__inlines_md5() -> None:
# Given a PERCENTAGE_SPLIT on `$.identity.identifier` and a context identity
# without `identifier`
seg: SegmentContext = {
"key": "ps4",
"name": "s",
Expand All @@ -600,8 +623,17 @@ def test_translate_segment__percentage_split_identity_identifier_missing__compil
],
}

# When / Then the predicate collapses to FALSE
assert translate_segment(seg, _ctx_identity_without("identifier")) == "((FALSE))"
# When / Then it still hashes the per-row identifier column
sql = translate_segment(seg, _ctx_identity_without("identifier"))
assert sql == (
"(((modulo(reinterpretAsUInt32(reverse(substring(MD5('ps4' || ',' ||"
" (toString(i.identifier))), 1, 4))) * 7291 +"
" reinterpretAsUInt32(reverse(substring(MD5('ps4' || ',' || (toString(i.identifier))),"
" 5, 4))) * 1897 + reinterpretAsUInt32(reverse(substring(MD5('ps4' || ',' ||"
" (toString(i.identifier))), 9, 4))) * 6835 +"
" reinterpretAsUInt32(reverse(substring(MD5('ps4' || ',' || (toString(i.identifier))),"
" 13, 4))), 9999) / 9998.0 * 100.0 <= 50.0)))"
)


def test_translate_segment__percentage_split_unknown_jsonpath__returns_none() -> None:
Expand Down
Loading