From 731af8d21f976aa86aaf18247d872f89d737884e Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Thu, 6 Aug 2026 23:31:32 +0200 Subject: [PATCH] fix(gooddata-eval): make MAQL comparison case-insensitive for keywords _normalize_maql/_best_maql_match compare an agent's generated MAQL against expected_output.maql via exact string equality after whitespace/wrapper normalization -- but MAQL keywords (SELECT, FOR PREVIOUS, WHERE, BY, ...) are case-insensitive at the query-engine level (confirmed against the MAQL reference), while the comparison itself was fully case-sensitive. Reproduced live in gdc-mic-ai-evaluation, post the #1718 fix: fixture "Create a metric for the prior-year value of Active cards" expects SELECT {metric/active_card_count_-_txn_-_cutcgco} FOR Previous({label/process_date.year}) Agent produced, verbatim: SELECT {metric/active_card_count_-_txn_-_cutcgco} FOR PREVIOUS({label/process_date.year}) Byte-identical except FOR PREVIOUS vs FOR Previous -- scored as a fail. First fix attempt considered and rejected: lowercase everything outside {type/id} braces. That's wrong -- WHERE-clause literal values are ALSO outside braces (e.g. WHERE {label/status} = "Active") and are real, case-sensitive data, not keywords; blindly folding them would create a new false-positive risk (two genuinely different filter values scored as equal). Actual fix: per the MAQL reference, every literal value is quoted and every identifier lives inside {..} -- both are exhaustively structural markers, so protecting text inside either while casefolding everything else needs no keyword list at all (which would risk being incomplete against MAQL's large vocabulary: SELECT, BY, WHERE, HAVING, FOR PREVIOUS/NEXT/EACH, WITHOUT PF, TOP/BOTTOM, WITHIN, RANK family, RUNSUM family, IFNULL, CASE/WHEN, 15+ math functions, ...). Added _casefold_outside_protected(), applied as the final step in _normalize_maql. Tests added: - keyword case-insensitivity on the exact reproduced case (FOR PREVIOUS vs FOR Previous) - identifier case preserved ({metric/Mixed_Case_Id} untouched) - quoted literal case preserved AND still distinguishes real differences (WHERE x = "Active" vs WHERE x = "active" must stay a genuine mismatch -- this is the test that would have caught the rejected first draft) Updated the one existing test whose expected value assumed no case normalization ever happens (SELECT -> select). Full gooddata-eval suite: 274 passed, 9 pre-existing unrelated failures (missing openai extra in this test env; two unrelated test files) -- identical count to before this change. Co-Authored-By: Claude Sonnet 5 --- .../core/agentic/metric_skill.py | 23 +++++++++++++++-- .../tests/test_agentic_metric_skill.py | 25 ++++++++++++++++++- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py index 2e2b5b9b1..652e47e6c 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py @@ -25,6 +25,12 @@ _IFNULL_RE = re.compile(r"IFNULL\s*\([^,]+,\s*0\)", re.IGNORECASE) _SELECT_WRAP_RE = re.compile(r"^\s*\(\s*SELECT\s*\{([^}]+)\}\s*\)\s*$", re.IGNORECASE) _INNER_SELECT_RE = re.compile(r"\(\s*SELECT\s*\{([^}]+)\}\s*\)", re.IGNORECASE) +# Matches whichever comes first: a {type/id} identifier reference or a quoted string +# literal -- both are case-sensitive data and must survive casefolding untouched. +# Everything else in MAQL (keywords, operators, numbers, punctuation) carries no +# case-sensitive meaning, per the MAQL reference (SELECT/BY/WHERE/FOR PREVIOUS/etc. +# are case-insensitive; only {..} identifiers and quoted literal values are not). +_PROTECTED_RE = re.compile(r"\{[^}]*\}|\"[^\"]*\"|'[^']*'") def _strip_outer_parens(s: str) -> str: @@ -42,8 +48,21 @@ def _strip_outer_parens(s: str) -> str: return s[1:-1].strip() +def _casefold_outside_protected(s: str) -> str: + """Lowercase MAQL keywords/operators while preserving case-sensitive {type/id} + identifiers and quoted string literal values (e.g. WHERE {label/x} = "Active").""" + parts = [] + last = 0 + for m in _PROTECTED_RE.finditer(s): + parts.append(s[last : m.start()].lower()) + parts.append(m.group(0)) + last = m.end() + parts.append(s[last:].lower()) + return "".join(parts) + + def _normalize_maql(maql: str) -> str: - """Semantic normalisation: strip whitespace, unwrap IFNULL/SELECT wrappers.""" + """Semantic normalisation: strip whitespace, unwrap IFNULL/SELECT wrappers, casefold keywords.""" if not maql: return "" m = maql.strip() @@ -56,7 +75,7 @@ def _normalize_maql(maql: str) -> str: m = re.sub(r"\{\s+", "{", m) m = re.sub(r"\s+\}", "}", m) m = re.sub(r"\s+", " ", m) - return m.strip() + return _casefold_outside_protected(m.strip()) def _best_maql_match(actual_maql: str, expected_outputs: list[dict]) -> tuple[bool, str]: diff --git a/packages/gooddata-eval/tests/test_agentic_metric_skill.py b/packages/gooddata-eval/tests/test_agentic_metric_skill.py index 67a163e92..7081abbef 100644 --- a/packages/gooddata-eval/tests/test_agentic_metric_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_metric_skill.py @@ -14,13 +14,36 @@ def test_normalize_maql_strips_whitespace(): - assert _normalize_maql(" SELECT { metric/foo } ") == "SELECT {metric/foo}" + assert _normalize_maql(" SELECT { metric/foo } ") == "select {metric/foo}" def test_normalize_maql_removes_select_wrapper(): assert _normalize_maql("(SELECT {metric/abc})") == "{metric/abc}" +def test_normalize_maql_is_case_insensitive_for_keywords(): + """Regression test for a live-reproduced bug: 'FOR PREVIOUS(...)' vs + 'FOR Previous(...)' scored as a mismatch even though MAQL keywords are + case-insensitive -- a semantically identical agent answer failed the eval + purely on keyword casing.""" + actual = "SELECT {metric/active_card_count_-_txn_-_cutcgco} FOR PREVIOUS({label/process_date.year})" + expected = "SELECT {metric/active_card_count_-_txn_-_cutcgco}\n FOR Previous({label/process_date.year})" + assert _normalize_maql(actual) == _normalize_maql(expected) + + +def test_normalize_maql_preserves_identifier_case(): + # {type/id} references are real, case-sensitive ids -- must never be casefolded. + assert "Mixed_Case_Id" in _normalize_maql("SELECT {metric/Mixed_Case_Id}") + + +def test_normalize_maql_preserves_quoted_literal_case(): + """The bug this guards against: naively lowercasing everything outside {..} + would also lowercase quoted WHERE-clause literal values, which are real, + case-sensitive data -- not keywords. Two literals differing only in case + must NOT be treated as equal; that would be a false positive.""" + assert _normalize_maql('WHERE {label/status} = "Active"') != _normalize_maql('WHERE {label/status} = "active"') + + def test_metric_run_result_fields(): r = MetricRunResult( conversation_id="c1",