diff --git a/packages/gooddata-eval/src/gooddata_eval/core/scoring.py b/packages/gooddata-eval/src/gooddata_eval/core/scoring.py index 0d554cd85..33bf6a630 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/scoring.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/scoring.py @@ -163,7 +163,25 @@ 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. + # + # 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 873e30628..75ea4499c 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,84 @@ 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 + + +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