From c265cc79e0d73b77c8a8f564391d77ef0646666b Mon Sep 17 00:00:00 2001 From: "my.nguyen" Date: Wed, 5 Aug 2026 08:08:34 +0700 Subject: [PATCH] fix(gooddata-eval): make ranking attribute optional on 1-dim viz The visualization comparator required a ranking filter's `attribute` to match exactly, but `attribute` is optional in the AAC schema -- gen-ai models it as `NotRequired[str]` / `str | None` in all three of its ranking-filter types, and when it is absent AFM ranks over every dimension of the result. On a chart with exactly one dimension that is the same filter, so the comparator was stricter than the product contract and failed correct answers. Both Anthropic models consistently omit `attribute` while getting the metric and top/bottom-N right, which made this the largest visualization failure cluster: 14 of 49 viz failures in run 30850362312 (opus48 9, sonnet46 3, bedrock 2). No GPT combo is affected. `_normalize_ranking_filter` now fills an omitted attribute in with the visualization's sole dimension URI instead of comparing it as an empty string. The substitution is gated on there being exactly ONE distinct dimension: with two or more, omitting `attribute` ranks over the dimension tuple, which is a genuinely different filter, so those stay strict. It is applied to expected and actual alike, because datasets omit `attribute` too -- without symmetry an agent that supplies the more precise filter would fail against a fixture that omits it. Missing, None and "" now normalize identically, so `attribute: null` no longer differs from an absent key. Also make `validate_cross_references` return a score instead of raising. None, "" and non-string values reached `.startswith()` / `dict.get()` and blew up with AttributeError / TypeError mid-evaluation. This affected the `using` branch as well as `attribute`. Its test asserts the expected verdict per malformed case rather than comparing `ok` against the returned error-list length, which was a tautology against an implementation that returns exactly `len(errors) == 0`; confirmed non-vacuous by mutation. Verified by re-scoring all 48 expected/actual pairs lifted from run 30850362312 with the patched module: 14 flip FAIL -> PASS, 0 checks that CI reported as True became False. Note this raises opus48's pass rate ~4.5pp for comparator reasons, not model ones. JIRA: QA-28615 risk: nonprod Co-Authored-By: Claude Opus 5 (1M context) --- .../src/gooddata_eval/core/scoring.py | 90 +++++++++++++---- packages/gooddata-eval/tests/test_scoring.py | 97 +++++++++++++++++++ .../tests/test_visualization_evaluator.py | 28 ++++++ 3 files changed, 195 insertions(+), 20 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/scoring.py b/packages/gooddata-eval/src/gooddata_eval/core/scoring.py index 816db306c..a5913a791 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/scoring.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/scoring.py @@ -63,29 +63,46 @@ def uri_to_display_name(uri: str) -> str: def validate_cross_references(viz: CreatedVisualization) -> tuple[bool, list[str]]: - """Validate ranking-filter `using`/`attribute` resolve to correct URI prefixes.""" + """Validate ranking-filter `using`/`attribute` resolve to correct URI prefixes. + + Always returns `(ok, errors)` — a malformed filter produces an error entry, never an + exception. Anything unusable (None, empty, non-string) used to reach `.startswith()` + or `dict.get()` and blow up with AttributeError/TypeError mid-evaluation. + + `using` is required by the AAC schema, `attribute` is optional (see + `_normalize_ranking_filter`), so an absent/None/empty `attribute` is accepted silently. + """ errors: list[str] = [] fields = viz.query.fields for filter_key, filter_dict in viz.query.filter_by.items(): if filter_dict.get("type") != "ranking_filter": continue - using_val = filter_dict.get("using", "") - using_uri = _resolve_alias_to_uri(using_val, fields) - field_def = fields.get(using_val) - is_adhoc_agg = isinstance(field_def, AacQueryField) and bool(field_def.aggregation) - if not using_uri.startswith(("metric/", "fact/")) and not is_adhoc_agg: - errors.append( - f"ranking filter '{filter_key}': using='{using_val}' " - f"resolves to '{using_uri}' — expected a metric/ or fact/ URI" - ) - if "attribute" in filter_dict: - attr_val = filter_dict["attribute"] - attr_uri = _resolve_alias_to_uri(attr_val, fields) - if not attr_uri.startswith(("label/", "attribute/")): + using_val = filter_dict.get("using") + if not isinstance(using_val, str) or not using_val: + errors.append(f"ranking filter '{filter_key}': using={using_val!r} — a metric/ or fact/ URI is required") + else: + using_uri = _resolve_alias_to_uri(using_val, fields) + field_def = fields.get(using_val) + is_adhoc_agg = isinstance(field_def, AacQueryField) and bool(field_def.aggregation) + if not using_uri.startswith(("metric/", "fact/")) and not is_adhoc_agg: errors.append( - f"ranking filter '{filter_key}': attribute='{attr_val}' " - f"resolves to '{attr_uri}' — expected a label/ or attribute/ URI" + f"ranking filter '{filter_key}': using='{using_val}' " + f"resolves to '{using_uri}' — expected a metric/ or fact/ URI" ) + attr_val = filter_dict.get("attribute") + if attr_val is None or attr_val == "": + continue + if not isinstance(attr_val, str): + errors.append( + f"ranking filter '{filter_key}': attribute={attr_val!r} — expected a label/ or attribute/ URI" + ) + continue + attr_uri = _resolve_alias_to_uri(attr_val, fields) + if not attr_uri.startswith(("label/", "attribute/")): + errors.append( + f"ranking filter '{filter_key}': attribute='{attr_val}' " + f"resolves to '{attr_uri}' — expected a label/ or attribute/ URI" + ) return len(errors) == 0, errors @@ -99,11 +116,43 @@ def _normalize_date_filter(filter_dict: dict, _fields: dict) -> dict: } -def _normalize_ranking_filter(filter_dict: dict, fields: dict[str, AacQueryField | str]) -> dict: +def _sole_dimension_uri(viz: CreatedVisualization) -> str | None: + """URI of the visualization's only dimension, or None when it has zero or several.""" + dim_uris = get_dimension_uri_set(viz) + return next(iter(dim_uris)) if len(dim_uris) == 1 else None + + +def _normalize_ranking_filter( + filter_dict: dict, + fields: dict[str, AacQueryField | str], + sole_dim_uri: str | None = None, +) -> dict: + """Canonicalize a ranking filter so equivalent filters compare equal. + + `attribute` is optional in the AAC schema (gen-ai models it as `NotRequired[str]` / + `str | None`), and when it is omitted AFM ranks over every dimension of the result. For a + single-dimension visualization that is exactly "rank by that one dimension", so an omitted + attribute is filled in with `sole_dim_uri` instead of comparing as an empty string — the + agent and the dataset may legitimately express the same filter either way. + + The substitution is deliberately gated on there being exactly ONE dimension: with two or + more, omitting `attribute` ranks over the dimension *tuple*, which is a different filter, + so those stay strict. Callers pass the sole dimension of the visualization the filter + belongs to, which makes the comparison symmetric — it does not matter which side omitted it. + + Missing, None and "" are all treated as "not specified"; so is a non-string, which + `validate_cross_references` reports separately rather than crashing the comparison. + """ + attr_val = filter_dict.get("attribute") + if not isinstance(attr_val, str) or not attr_val: + dim_uri = sole_dim_uri or "" + else: + dim_uri = _resolve_alias_to_uri(attr_val, fields) + using_val = filter_dict.get("using") entry: dict = { "type": "ranking_filter", - "metric_uri": _resolve_alias_to_uri(filter_dict.get("using", ""), fields), - "dim_uri": _resolve_alias_to_uri(filter_dict.get("attribute", ""), fields), + "metric_uri": _resolve_alias_to_uri(using_val, fields) if isinstance(using_val, str) else "", + "dim_uri": dim_uri, } if "top" in filter_dict: entry["top"] = filter_dict["top"] @@ -127,12 +176,13 @@ def _split_and_normalize_filters(viz: CreatedVisualization) -> tuple[set[str], s ranking_set: set[str] = set() attr_set: set[str] = set() fields = viz.query.fields + sole_dim_uri = _sole_dimension_uri(viz) for filter_dict in viz.query.filter_by.values(): ft = filter_dict.get("type") if ft == "date_filter": date_set.add(json.dumps(_normalize_date_filter(filter_dict, fields), sort_keys=True)) elif ft == "ranking_filter": - ranking_set.add(json.dumps(_normalize_ranking_filter(filter_dict, fields), sort_keys=True)) + ranking_set.add(json.dumps(_normalize_ranking_filter(filter_dict, fields, sole_dim_uri), sort_keys=True)) elif ft == "attribute_filter": attr_set.add(json.dumps(_normalize_attribute_filter(filter_dict, fields), sort_keys=True)) return date_set, ranking_set, attr_set diff --git a/packages/gooddata-eval/tests/test_scoring.py b/packages/gooddata-eval/tests/test_scoring.py index f8bbeb4bb..15f16d383 100644 --- a/packages/gooddata-eval/tests/test_scoring.py +++ b/packages/gooddata-eval/tests/test_scoring.py @@ -64,3 +64,100 @@ def test_check_filters_exact_attribute_match(): actual = _viz(query={"fields": {}, "filter_by": f}) scores = check_filters(expected, actual) assert scores.all_ok is True + + +# --- 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 +# absent, so on a one-dimension chart "omitted" and "the sole dimension" mean the same filter. +# The comparator used to demand an exact match and failed those as filters_correct=False. + +_M = {"m_sales": {"using": "metric/net_sales"}} +# same URI behind two different aliases — normalization must be alias-independent +_ONE_DIM_A = {**_M, "d_product_id": {"using": "label/product_id"}} +_ONE_DIM_B = {**_M, "d_product": {"using": "label/product_id"}} +_TWO_DIM = {**_M, "d_brand": {"using": "label/product_brand"}, "d_city": {"using": "label/customer_city"}} + + +def _rank_viz(fields, dims, **filter_overrides): + rank = {"type": "ranking_filter", "using": "m_sales", "top": 1, **filter_overrides} + return _viz( + type="bar_chart", + query={"fields": fields, "filter_by": {"f_rank": rank}}, + metrics=["m_sales"], + view_by=dims, + ) + + +def test_ranking_attribute_optional_on_single_dimension_viz(): + """Expected names the attribute, actual omits it — one dimension, so they are equivalent.""" + expected = _rank_viz(_ONE_DIM_A, ["d_product_id"], attribute="d_product_id") + actual = _rank_viz(_ONE_DIM_B, ["d_product"]) + scores = check_filters(expected, actual) + assert scores.ranking_ok is True + assert scores.all_ok is True + + +def test_ranking_attribute_optional_is_symmetric(): + """Reverse direction: the dataset omits the attribute and the agent supplies it.""" + expected = _rank_viz(_ONE_DIM_A, ["d_product_id"]) + actual = _rank_viz(_ONE_DIM_B, ["d_product"], attribute="d_product") + assert check_filters(expected, actual).ranking_ok is True + + +def test_ranking_attribute_none_and_empty_are_the_same_as_omitted(): + expected = _rank_viz(_ONE_DIM_A, ["d_product_id"], attribute="d_product_id") + for omitted in ({"attribute": None}, {"attribute": ""}): + actual = _rank_viz(_ONE_DIM_B, ["d_product"], **omitted) + assert check_filters(expected, actual).ranking_ok is True, omitted + + +def test_ranking_attribute_still_required_on_multi_dimension_viz(): + """Two dimensions: omitting the attribute ranks over the tuple, so it stays strict.""" + expected = _rank_viz(_TWO_DIM, ["d_brand", "d_city"], attribute="d_brand") + actual = _rank_viz(_TWO_DIM, ["d_brand", "d_city"]) + assert check_filters(expected, actual).ranking_ok is False + + +def test_ranking_attribute_omitted_does_not_mask_a_wrong_top_n(): + expected = _rank_viz(_ONE_DIM_A, ["d_product_id"], attribute="d_product_id", top=1) + actual = _rank_viz(_ONE_DIM_B, ["d_product"], top=5) + assert check_filters(expected, actual).ranking_ok is False + + +def test_ranking_attribute_omitted_does_not_mask_a_wrong_dimension(): + expected = _rank_viz(_ONE_DIM_A, ["d_product_id"], attribute="d_product_id") + actual = _rank_viz(_TWO_DIM, ["d_brand"]) # single dim, but a different one + assert check_filters(expected, actual).ranking_ok is False + + +def test_validate_cross_references_never_raises_on_empty_or_none_uris(): + """Each of these used to raise AttributeError/TypeError instead of returning a score. + + Every case carries its expected verdict: `attribute` is optional so None/"" are valid, + while a non-string attribute or a missing/None `using` must be reported as an error. + Asserting the verdict is what stops a malformed filter from silently passing as valid. + """ + cases = [ + ({"type": "ranking_filter", "using": "m_sales", "top": 5, "attribute": None}, True), + ({"type": "ranking_filter", "using": "m_sales", "top": 5, "attribute": ""}, True), + ({"type": "ranking_filter", "using": "m_sales", "top": 5, "attribute": []}, False), + ({"type": "ranking_filter", "using": None, "top": 5}, False), + ({"type": "ranking_filter", "top": 5}, False), + ] + for rank, expected_ok in cases: + viz = _viz(query={"fields": _M, "filter_by": {"f_rank": rank}}) + ok, errors = validate_cross_references(viz) + assert isinstance(ok, bool) and isinstance(errors, list), rank + assert ok is expected_ok, rank + assert bool(errors) is not expected_ok, rank + + +def test_validate_cross_references_accepts_omitted_attribute_but_flags_missing_using(): + omitted = _viz(query={"fields": _M, "filter_by": {"f": {"type": "ranking_filter", "using": "m_sales", "top": 5}}}) + assert validate_cross_references(omitted) == (True, []) + + no_using = _viz(query={"fields": _M, "filter_by": {"f": {"type": "ranking_filter", "top": 5}}}) + ok, errors = validate_cross_references(no_using) + assert ok is False + assert "is required" in errors[0] diff --git a/packages/gooddata-eval/tests/test_visualization_evaluator.py b/packages/gooddata-eval/tests/test_visualization_evaluator.py index 7e77b60ab..38d89e844 100644 --- a/packages/gooddata-eval/tests/test_visualization_evaluator.py +++ b/packages/gooddata-eval/tests/test_visualization_evaluator.py @@ -99,3 +99,31 @@ def test_evaluator_skill_not_activated_when_wrong_skill_name(): ) result = ev.evaluate(_item(_expected()), chat) assert result.detail["skill_activated"] is False + + +def _ranked(attribute: str | None, dim_alias: str = "d_q"): + """Single-dimension chart with a top-1 ranking filter, optionally naming the attribute.""" + rank = {"type": "ranking_filter", "using": "m_rev", "top": 1} + if attribute is not None: + rank["attribute"] = attribute + return { + "id": "x", + "type": "column_chart", + "query": { + "fields": {"m_rev": {"using": "metric/revenue"}, dim_alias: {"using": "label/date.quarter"}}, + "filter_by": {"f_rank": rank}, + }, + "metrics": ["m_rev"], + "view_by": [dim_alias], + } + + +def test_evaluator_passes_when_agent_omits_ranking_attribute_on_single_dim_viz(): + """QA-28615: the omitted attribute resolves to the sole dimension, so the case must pass.""" + ev = get_evaluator("visualization") + expected = _ranked("d_q") + actual = _ranked(None, dim_alias="d_quarter") # different alias, attribute omitted + result = ev.evaluate(_item(expected), _chat_result_with(actual)) + assert result.detail["filter_ranking_score"] is True + assert result.detail["filters_correct"] is True + assert result.passed is True