From b32a0d2901354b46a40a7afad9390aaea77263b6 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Thu, 10 Sep 2026 14:06:08 +0200 Subject: [PATCH 1/2] fix(gooddata-eval): compare attribute-filter elements as a set, not a sequence An attribute filter selecting exactly the same elements scored as a mismatch when the agent emitted them in a different order than the fixture listed them. _split_and_normalize_filters serialises each normalised filter with json.dumps(..., sort_keys=True). That orders the dict KEYS -- field_uri, state, type -- and never descends into the list under state["include"], so the two serialisations differed and check_filters compared them with ==. `include`/`exclude` name a SET of elements. An agent has no reason to keep their order stable between runs, so any question needing a multi-element attribute filter passed or failed partly at random -- non-determinism inside scoring, reported as `filters_correct: false` and indistinguishable from the agent genuinely filtering wrongly. Found on what-is-cross-border-approval-rate-in-the (micai_diagnose_master, gpt-5.2): metrics and dimensions correct, filters_correct false, with expected ["Inter-region", "Intra-region"] against actual ["Intra-region", "Inter-region"]. Both select every non-Domestic, non-Unknown row. Sorting during normalisation rather than comparing as sets keeps the canonical JSON string that normalized_filters reports for debugging, and keeps both sides in one place. `key=str` because a mixed-type list would raise TypeError from inside scoring, which is worse than the mismatch this fixes; malformed filter values are reported by validate_cross_references separately. The other two normalisers were checked and are unaffected: _normalize_ranking_filter and _normalize_date_filter emit only scalars, so state's element lists are the only ordered value in filter normalisation. Multiple attribute filters already compared order-insensitively, since _split_and_normalize_filters collects entries into a set -- the set was right, the list inside each member was not. 7 tests, 6 of which fail against the previous version; the 7th is the negative control that a genuinely different element set still fails. 921 passed, lint and format clean. Co-Authored-By: Claude Opus 5 --- .../src/gooddata_eval/core/scoring.py | 14 +- packages/gooddata-eval/tests/test_scoring.py | 138 ++++++++++++++++++ 2 files changed, 151 insertions(+), 1 deletion(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/scoring.py b/packages/gooddata-eval/src/gooddata_eval/core/scoring.py index 0d554cd85..126193927 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/scoring.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/scoring.py @@ -163,7 +163,19 @@ def _normalize_ranking_filter( def _normalize_attribute_filter(filter_dict: dict, _fields: dict) -> dict: raw_state = filter_dict.get("state") or {} - state = {k: v for k, v in raw_state.items() if v} + # Sort the element lists: `include`/`exclude` name a SET of elements, but the caller + # serialises this dict with json.dumps(..., sort_keys=True), which orders the dict + # KEYS and leaves the lists alone. Without this, the same filter written in a + # different order compares unequal, and an agent has no reason to keep that order + # stable between runs -- so a question needing a multi-element filter passed or + # failed partly at random, reported as `filters_correct: false` and indistinguishable + # from the agent genuinely filtering wrongly. + # + # `key=str` rather than a bare sort: a mixed-type list (["A", 2]) would raise + # TypeError from inside scoring, which is worse than the mismatch this fixes. + # validate_cross_references reports malformed filter values separately, so this only + # has to avoid crashing on them. + state = {k: (sorted(v, key=str) if isinstance(v, list) else v) for k, v in raw_state.items() if v} return { "type": "attribute_filter", "field_uri": filter_dict.get("using", ""), diff --git a/packages/gooddata-eval/tests/test_scoring.py b/packages/gooddata-eval/tests/test_scoring.py index 873e30628..0826ffd9d 100644 --- a/packages/gooddata-eval/tests/test_scoring.py +++ b/packages/gooddata-eval/tests/test_scoring.py @@ -67,6 +67,80 @@ def test_check_filters_exact_attribute_match(): assert scores.all_ok is True +# --- attribute-filter element order must not decide the verdict --- +# +# `_normalize_attribute_filter` passes `state` through untouched and the caller serialises +# it with json.dumps(..., sort_keys=True). sort_keys orders the DICT KEYS (field_uri, +# state, type) and never the LIST under state["include"], so two filters selecting the +# same elements in a different order compare unequal. +# +# Found from a real eval run (gdc-mic-ai-evaluation, micai_diagnose_master, 2026-09-10): +# a question filtering cross-border traffic scored metrics_correct=True, +# dimensions_correct=True, filters_correct=False, because the fixture listed +# ["Inter-region", "Intra-region"] and the agent emitted ["Intra-region", "Inter-region"]. +# Element order is not something an agent has any reason to keep stable between runs, so +# every question needing a multi-value attribute filter passes or fails partly at random. + + +def test_attribute_filter_include_order_does_not_change_the_verdict(): + def viz(values): + return _viz( + query={ + "fields": {}, + "filter_by": { + "f_a": { + "type": "attribute_filter", + "using": "label/cross_border_name", + "state": {"include": values}, + } + }, + } + ) + + expected = viz(["Inter-region", "Intra-region"]) + actual = viz(["Intra-region", "Inter-region"]) + assert check_filters(expected, actual).attribute_ok is True + + +def test_attribute_filter_exclude_order_does_not_change_the_verdict(): + def viz(values): + return _viz( + query={ + "fields": {}, + "filter_by": { + "f_a": { + "type": "attribute_filter", + "using": "label/region", + "state": {"exclude": values}, + } + }, + } + ) + + assert check_filters(viz(["EMEA", "APAC"]), viz(["APAC", "EMEA"])).attribute_ok is True + + +def test_attribute_filter_with_different_elements_still_fails(): + """The fix must not make the comparison permissive -- a genuinely different set + of elements is still a mismatch.""" + + def viz(values): + return _viz( + query={ + "fields": {}, + "filter_by": { + "f_a": { + "type": "attribute_filter", + "using": "label/region", + "state": {"include": values}, + } + }, + } + ) + + assert check_filters(viz(["EMEA", "APAC"]), viz(["EMEA", "LATAM"])).attribute_ok is False + + # --- ranking-filter `attribute` is optional on single-dimension visualizations (QA-28615) --- # # `attribute` is NotRequired in the AAC schema and AFM ranks over the whole result when it is @@ -205,3 +279,67 @@ def test_normalized_filters_is_empty_per_category_when_unfiltered(): } ) assert normalized_filters(viz) == {"date": [], "ranking": [], "attribute": []} + + +def _attr_viz(values, key="include", using="label/cross_border_name"): + return _viz( + query={ + "fields": {"m": {"using": "metric/approval_rate"}}, + "filter_by": {"f": {"type": "attribute_filter", "using": using, "state": {key: values}}}, + }, + metrics=["m"], + ) + + +def test_attribute_filter_elements_compare_as_a_set_not_a_sequence(): + """`include`/`exclude` name a set of elements, so element order must not decide a verdict. + + json.dumps(sort_keys=True) orders the dict KEYS and leaves the lists alone, so the same + filter emitted in a different order compared unequal -- and the agent has no reason to + keep that order stable between runs. The failure reported as `filters_correct: false`, + indistinguishable from the agent genuinely filtering wrongly. + """ + expected = _attr_viz(["Inter-region", "Intra-region"]) + assert check_filters(expected, _attr_viz(["Inter-region", "Intra-region"])).attribute_ok is True + assert check_filters(expected, _attr_viz(["Intra-region", "Inter-region"])).attribute_ok is True + + +def test_a_three_element_attribute_filter_is_order_insensitive(): + """Two elements need 2 permutations, three need 6 -- admitting them as extra fixture + candidates grows factorially, which is why this belongs in normalisation.""" + expected = _attr_viz(["A", "B", "C"]) + for actual in (["C", "A", "B"], ["B", "C", "A"], ["C", "B", "A"]): + assert check_filters(expected, _attr_viz(actual)).attribute_ok is True + + +def test_exclude_elements_are_order_insensitive_too(): + expected = _attr_viz(["Domestic", "Unknown"], key="exclude") + assert check_filters(expected, _attr_viz(["Unknown", "Domestic"], key="exclude")).attribute_ok is True + + +def test_ordering_does_not_mask_a_genuinely_different_element_set(): + """The guard against the fix being "pass everything": different elements still fail.""" + expected = _attr_viz(["Inter-region", "Intra-region"]) + assert check_filters(expected, _attr_viz(["Inter-region"])).attribute_ok is False + assert check_filters(expected, _attr_viz(["Inter-region", "Domestic"])).attribute_ok is False + + +def test_include_and_exclude_of_the_same_elements_still_differ(): + """Sorting must not collapse the two state keys into each other.""" + inc = _attr_viz(["Domestic", "Unknown"], key="include") + exc = _attr_viz(["Unknown", "Domestic"], key="exclude") + assert check_filters(inc, exc).attribute_ok is False + + +def test_the_same_elements_on_a_different_label_still_differ(): + expected = _attr_viz(["A", "B"], using="label/cross_border_name") + assert check_filters(expected, _attr_viz(["B", "A"], using="label/region_name")).attribute_ok is False + + +def test_a_mixed_type_element_list_does_not_crash_scoring(): + """A malformed list would raise TypeError from a bare sorted(), and a crash inside + scoring is worse than the mismatch this fixes. validate_cross_references reports + malformed filter values separately, so this only has to stay comparable.""" + expected = _attr_viz(["A", 2]) + assert check_filters(expected, _attr_viz([2, "A"])).attribute_ok is True + assert check_filters(expected, _attr_viz(["A", 3])).attribute_ok is False From a1efb45c838f5c70a7aa07a1074566e75babecb2 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Thu, 10 Sep 2026 14:18:13 +0200 Subject: [PATCH 2/2] fix(gooddata-eval): sort filter elements by canonical JSON, not str() Review finding, and correct. `key=str` maps 1 and "1" to the same key, so Python's stable sort leaves THEIR relative order exactly as the agent emitted it -- and the ordering bug this PR fixes survives for that one pair. [1, "1"] and ["1", 1] still serialised differently and still scored as different filters. The key is now the element's own canonical JSON. That keeps what str() was chosen for -- a mixed-type list must not raise TypeError from inside scoring -- while distinguishing the types the comparison downstream also distinguishes. Safe here because these values are always parsed JSON, so json.dumps cannot fail on them; that provenance is what makes it a total ordering. Two tests: the type-collision pair now compares equal without making 1 and "1" interchangeable as element sets, and a heterogeneous list sorts without raising. The first fails against key=str. 923 passed, lint and format clean. Co-Authored-By: Claude Opus 5 --- .../src/gooddata_eval/core/scoring.py | 16 +++++++++++----- packages/gooddata-eval/tests/test_scoring.py | 17 +++++++++++++++++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/scoring.py b/packages/gooddata-eval/src/gooddata_eval/core/scoring.py index 126193927..33bf6a630 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/scoring.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/scoring.py @@ -171,11 +171,17 @@ def _normalize_attribute_filter(filter_dict: dict, _fields: dict) -> dict: # failed partly at random, reported as `filters_correct: false` and indistinguishable # from the agent genuinely filtering wrongly. # - # `key=str` rather than a bare sort: a mixed-type list (["A", 2]) would raise - # TypeError from inside scoring, which is worse than the mismatch this fixes. - # validate_cross_references reports malformed filter values separately, so this only - # has to avoid crashing on them. - state = {k: (sorted(v, key=str) if isinstance(v, list) else v) for k, v in raw_state.items() if v} + # The key is the element's own canonical JSON, not a bare sort and not str(): a bare + # sort raises TypeError on a mixed-type list (["A", 2]), and a crash inside scoring is + # worse than the mismatch this fixes -- while str() collapses 1 and "1" to the same + # key, so the stable sort leaves THEIR order as it found it and the ordering bug + # survives for exactly that pair. These values are always parsed JSON, so json.dumps + # cannot fail on them and it distinguishes types the way the comparison downstream does. + state = { + k: (sorted(v, key=lambda element: json.dumps(element, sort_keys=True)) if isinstance(v, list) else v) + for k, v in raw_state.items() + if v + } return { "type": "attribute_filter", "field_uri": filter_dict.get("using", ""), diff --git a/packages/gooddata-eval/tests/test_scoring.py b/packages/gooddata-eval/tests/test_scoring.py index 0826ffd9d..75ea4499c 100644 --- a/packages/gooddata-eval/tests/test_scoring.py +++ b/packages/gooddata-eval/tests/test_scoring.py @@ -343,3 +343,20 @@ def test_a_mixed_type_element_list_does_not_crash_scoring(): expected = _attr_viz(["A", 2]) assert check_filters(expected, _attr_viz([2, "A"])).attribute_ok is True assert check_filters(expected, _attr_viz(["A", 3])).attribute_ok is False + + +def test_elements_that_stringify_alike_but_differ_in_type_still_sort_stably(): + """`key=str` collapsed 1 and "1" to the same sort key, so Python's stable sort left + their relative order exactly as the agent emitted it and the ordering bug survived for + that pair alone. The key is the element's canonical JSON instead, which distinguishes + the types the comparison downstream also distinguishes.""" + assert check_filters(_attr_viz([1, "1"]), _attr_viz(["1", 1])).attribute_ok is True + # ...without making the two types interchangeable: one element is not the other set. + assert check_filters(_attr_viz([1]), _attr_viz(["1"])).attribute_ok is False + + +def test_heterogeneous_element_lists_sort_without_raising(): + """Every value here is parsed JSON, so json.dumps cannot fail on it -- which is what + makes it usable as a total ordering where a bare sort would raise.""" + mixed = [None, True, 2, "a", 1.5] + assert check_filters(_attr_viz(mixed), _attr_viz(list(reversed(mixed)))).attribute_ok is True