Skip to content

Commit 7fcd55c

Browse files
Merge pull request #1822 from gooddata/snapshot-master-c73f047c-to-rel/dev
[bot] Merge master/c73f047c into rel/dev
2 parents be8934a + c73f047 commit 7fcd55c

2 files changed

Lines changed: 174 additions & 1 deletion

File tree

‎packages/gooddata-eval/src/gooddata_eval/core/scoring.py‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,25 @@ def _normalize_ranking_filter(
246246

247247
def _normalize_attribute_filter(filter_dict: dict, _fields: dict) -> dict:
248248
raw_state = filter_dict.get("state") or {}
249-
state = {k: v for k, v in raw_state.items() if v}
249+
# Sort the element lists: `include`/`exclude` name a SET of elements, but the caller
250+
# serialises this dict with json.dumps(..., sort_keys=True), which orders the dict
251+
# KEYS and leaves the lists alone. Without this, the same filter written in a
252+
# different order compares unequal, and an agent has no reason to keep that order
253+
# stable between runs -- so a question needing a multi-element filter passed or
254+
# failed partly at random, reported as `filters_correct: false` and indistinguishable
255+
# from the agent genuinely filtering wrongly.
256+
#
257+
# The key is the element's own canonical JSON, not a bare sort and not str(): a bare
258+
# sort raises TypeError on a mixed-type list (["A", 2]), and a crash inside scoring is
259+
# worse than the mismatch this fixes -- while str() collapses 1 and "1" to the same
260+
# key, so the stable sort leaves THEIR order as it found it and the ordering bug
261+
# survives for exactly that pair. These values are always parsed JSON, so json.dumps
262+
# cannot fail on them and it distinguishes types the way the comparison downstream does.
263+
state = {
264+
k: (sorted(v, key=lambda element: json.dumps(element, sort_keys=True)) if isinstance(v, list) else v)
265+
for k, v in raw_state.items()
266+
if v
267+
}
250268
return {
251269
"type": "attribute_filter",
252270
"field_uri": filter_dict.get("using", ""),

‎packages/gooddata-eval/tests/test_scoring.py‎

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,80 @@ def test_check_filters_exact_attribute_match():
7070
assert scores.all_ok is True
7171

7272

73+
# --- attribute-filter element order must not decide the verdict ---
74+
#
75+
# `_normalize_attribute_filter` passes `state` through untouched and the caller serialises
76+
# it with json.dumps(..., sort_keys=True). sort_keys orders the DICT KEYS (field_uri,
77+
# state, type) and never the LIST under state["include"], so two filters selecting the
78+
# same elements in a different order compare unequal.
79+
#
80+
# Found from a real eval run (gdc-mic-ai-evaluation, micai_diagnose_master, 2026-09-10):
81+
# a question filtering cross-border traffic scored metrics_correct=True,
82+
# dimensions_correct=True, filters_correct=False, because the fixture listed
83+
# ["Inter-region", "Intra-region"] and the agent emitted ["Intra-region", "Inter-region"].
84+
# Element order is not something an agent has any reason to keep stable between runs, so
85+
# every question needing a multi-value attribute filter passes or fails partly at random.
86+
87+
88+
def test_attribute_filter_include_order_does_not_change_the_verdict():
89+
def viz(values):
90+
return _viz(
91+
query={
92+
"fields": {},
93+
"filter_by": {
94+
"f_a": {
95+
"type": "attribute_filter",
96+
"using": "label/cross_border_name",
97+
"state": {"include": values},
98+
}
99+
},
100+
}
101+
)
102+
103+
expected = viz(["Inter-region", "Intra-region"])
104+
actual = viz(["Intra-region", "Inter-region"])
105+
assert check_filters(expected, actual).attribute_ok is True
106+
107+
108+
def test_attribute_filter_exclude_order_does_not_change_the_verdict():
109+
def viz(values):
110+
return _viz(
111+
query={
112+
"fields": {},
113+
"filter_by": {
114+
"f_a": {
115+
"type": "attribute_filter",
116+
"using": "label/region",
117+
"state": {"exclude": values},
118+
}
119+
},
120+
}
121+
)
122+
123+
assert check_filters(viz(["EMEA", "APAC"]), viz(["APAC", "EMEA"])).attribute_ok is True
124+
125+
126+
def test_attribute_filter_with_different_elements_still_fails():
127+
"""The fix must not make the comparison permissive -- a genuinely different set
128+
of elements is still a mismatch."""
129+
130+
def viz(values):
131+
return _viz(
132+
query={
133+
"fields": {},
134+
"filter_by": {
135+
"f_a": {
136+
"type": "attribute_filter",
137+
"using": "label/region",
138+
"state": {"include": values},
139+
}
140+
},
141+
}
142+
)
143+
144+
assert check_filters(viz(["EMEA", "APAC"]), viz(["EMEA", "LATAM"])).attribute_ok is False
145+
146+
73147
# --- ranking-filter `attribute` is optional on single-dimension visualizations (QA-28615) ---
74148
#
75149
# `attribute` is NotRequired in the AAC schema and AFM ranks over the whole result when it is
@@ -210,6 +284,87 @@ def test_normalized_filters_is_empty_per_category_when_unfiltered():
210284
assert normalized_filters(viz) == {"date": [], "ranking": [], "attribute": []}
211285

212286

287+
def _attr_viz(values, key="include", using="label/cross_border_name"):
288+
return _viz(
289+
query={
290+
"fields": {"m": {"using": "metric/approval_rate"}},
291+
"filter_by": {"f": {"type": "attribute_filter", "using": using, "state": {key: values}}},
292+
},
293+
metrics=["m"],
294+
)
295+
296+
297+
def test_attribute_filter_elements_compare_as_a_set_not_a_sequence():
298+
"""`include`/`exclude` name a set of elements, so element order must not decide a verdict.
299+
300+
json.dumps(sort_keys=True) orders the dict KEYS and leaves the lists alone, so the same
301+
filter emitted in a different order compared unequal -- and the agent has no reason to
302+
keep that order stable between runs. The failure reported as `filters_correct: false`,
303+
indistinguishable from the agent genuinely filtering wrongly.
304+
"""
305+
expected = _attr_viz(["Inter-region", "Intra-region"])
306+
assert check_filters(expected, _attr_viz(["Inter-region", "Intra-region"])).attribute_ok is True
307+
assert check_filters(expected, _attr_viz(["Intra-region", "Inter-region"])).attribute_ok is True
308+
309+
310+
def test_a_three_element_attribute_filter_is_order_insensitive():
311+
"""Two elements need 2 permutations, three need 6 -- admitting them as extra fixture
312+
candidates grows factorially, which is why this belongs in normalisation."""
313+
expected = _attr_viz(["A", "B", "C"])
314+
for actual in (["C", "A", "B"], ["B", "C", "A"], ["C", "B", "A"]):
315+
assert check_filters(expected, _attr_viz(actual)).attribute_ok is True
316+
317+
318+
def test_exclude_elements_are_order_insensitive_too():
319+
expected = _attr_viz(["Domestic", "Unknown"], key="exclude")
320+
assert check_filters(expected, _attr_viz(["Unknown", "Domestic"], key="exclude")).attribute_ok is True
321+
322+
323+
def test_ordering_does_not_mask_a_genuinely_different_element_set():
324+
"""The guard against the fix being "pass everything": different elements still fail."""
325+
expected = _attr_viz(["Inter-region", "Intra-region"])
326+
assert check_filters(expected, _attr_viz(["Inter-region"])).attribute_ok is False
327+
assert check_filters(expected, _attr_viz(["Inter-region", "Domestic"])).attribute_ok is False
328+
329+
330+
def test_include_and_exclude_of_the_same_elements_still_differ():
331+
"""Sorting must not collapse the two state keys into each other."""
332+
inc = _attr_viz(["Domestic", "Unknown"], key="include")
333+
exc = _attr_viz(["Unknown", "Domestic"], key="exclude")
334+
assert check_filters(inc, exc).attribute_ok is False
335+
336+
337+
def test_the_same_elements_on_a_different_label_still_differ():
338+
expected = _attr_viz(["A", "B"], using="label/cross_border_name")
339+
assert check_filters(expected, _attr_viz(["B", "A"], using="label/region_name")).attribute_ok is False
340+
341+
342+
def test_a_mixed_type_element_list_does_not_crash_scoring():
343+
"""A malformed list would raise TypeError from a bare sorted(), and a crash inside
344+
scoring is worse than the mismatch this fixes. validate_cross_references reports
345+
malformed filter values separately, so this only has to stay comparable."""
346+
expected = _attr_viz(["A", 2])
347+
assert check_filters(expected, _attr_viz([2, "A"])).attribute_ok is True
348+
assert check_filters(expected, _attr_viz(["A", 3])).attribute_ok is False
349+
350+
351+
def test_elements_that_stringify_alike_but_differ_in_type_still_sort_stably():
352+
"""`key=str` collapsed 1 and "1" to the same sort key, so Python's stable sort left
353+
their relative order exactly as the agent emitted it and the ordering bug survived for
354+
that pair alone. The key is the element's canonical JSON instead, which distinguishes
355+
the types the comparison downstream also distinguishes."""
356+
assert check_filters(_attr_viz([1, "1"]), _attr_viz(["1", 1])).attribute_ok is True
357+
# ...without making the two types interchangeable: one element is not the other set.
358+
assert check_filters(_attr_viz([1]), _attr_viz(["1"])).attribute_ok is False
359+
360+
361+
def test_heterogeneous_element_lists_sort_without_raising():
362+
"""Every value here is parsed JSON, so json.dumps cannot fail on it -- which is what
363+
makes it usable as a total ordering where a bare sort would raise."""
364+
mixed = [None, True, 2, "a", 1.5]
365+
assert check_filters(_attr_viz(mixed), _attr_viz(list(reversed(mixed)))).attribute_ok is True
366+
367+
213368
# --- relative vs absolute date filters denote the same period ---
214369
#
215370
# The agent answers "last month" either relatively (granularity MONTH, from -1, to -1)

0 commit comments

Comments
 (0)