-
Notifications
You must be signed in to change notification settings - Fork 70
fix(gooddata-eval): make MAQL comparison case-insensitive for keywords #1719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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"\{[^}]*\}|\"[^\"]*\"|'[^']*'") | ||
|
Comment on lines
+28
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 3 -F '\"' packages/gooddata-eval || true
rg -n -C 3 -F "\'" packages/gooddata-eval || trueRepository: gooddata/gooddata-python-sdk Length of output: 3673 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- metric_skill.py outline ---'
ast-grep outline packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py || true
printf '%s\n' '--- metric_skill.py relevant source ---'
cat -n packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py | sed -n '1,110p'
printf '%s\n' '--- metric skill tests ---'
fd -i -t f 'metric.*test|test.*metric' packages/gooddata-eval/tests packages/gooddata-eval || true
rg -n -C 4 '_casefold_outside_protected|_best_maql_match|PROTECTED_RE|escaped|casefold' packages/gooddata-eval/tests packages/gooddata-eval/src/gooddata_eval/core/agentic || true
printf '%s\n' '--- parser and MAQL literal references ---'
rg -n -C 3 'MAQL|quoted literal|escape|backslash|casefold' packages/gooddata-eval | head -300 || true
printf '%s\n' '--- static behavioral probe of the current and proposed regexes ---'
python3 - <<'PY'
import re
current = re.compile(r'\{[^}]*\}|"[^"]*"|\'[^\']*\'')
proposed = re.compile(r'\{[^}]*\}|"(?:\\.|[^"\\])*"|\'(?:\\.|[^\'\\])*\'')
samples = [
r'"Active"',
r'"Act\\"ive"', # backslash-escaped double quote in MAQL text
r'"Active\\\\"', # escaped backslash at the end of a literal
r"'Act\\'ive'", # backslash-escaped single quote
r'"Active" AND "Other"',
]
for sample in samples:
print('INPUT ', repr(sample))
for label, pattern in [('current', current), ('proposed', proposed)]:
matches = [(m.group(), m.span()) for m in pattern.finditer(sample)]
folded = pattern.sub(lambda m: m.group(), sample.casefold())
print(f'{label:8}', matches, 'casefold-preserving=', folded)
PYRepository: gooddata/gooddata-python-sdk Length of output: 32156 🌐 Web query:
💡 Result: In GoodData MAQL, string literals are typically enclosed in double quotation marks [1][2]. When a string contains characters that need to be escaped—such as internal double quotes—the backslash character () is used as the escape prefix [3]. For example, a string containing a single quote, such as "O'Brien", is rendered and escaped as "O'Brien" when used in a MAQL context [3]. The system automatically handles this escaping for parameters and string values to prevent syntax errors or the creation of broken string literals [3]. More broadly, in GoodData's underlying CTL (CloudConnect Transformation Language) literal syntax, double-quoted strings support the following backslash-escaped control characters [4]: - \n (newline) - \r (carriage return) - \t (tab) - \ (backslash) - " (double quote) - \b (backspace) If you are manually constructing expressions or interacting with GoodData via APIs where JSON or raw strings are required, ensure that double quotes within your string literals are preceded by a backslash (e.g., "") to maintain valid syntax [5]. Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- existing metric normalization tests ---'
cat -n packages/gooddata-eval/tests/test_agentic_metric_skill.py | sed -n '55,105p'
printf '%s\n' '--- exact escape-aware behavior for one escaped quote ---'
python3 - <<'PY'
import re
current = re.compile(r'\{[^}]*\}|"[^"]*"|\'[^\']*\'')
proposed = re.compile(r'\{[^}]*\}|"(?:\\.|[^"\\])*"|\'(?:\\.|[^\'\\])*\'')
samples = [
'"' + 'Act' + '\\' + '"' + 'ive"', # actual MAQL: "Act\"ive"
"'" + 'Act' + '\\' + "'" + "ive'", # actual MAQL: 'Act\'ive'
'"' + 'Active' + '\\\\' + '"', # actual MAQL: "Active\\"
'"' + 'Active' + '" AND "' + 'Other' + '"',
]
for sample in samples:
print('INPUT:', repr(sample))
for name, pattern in [('current', current), ('proposed', proposed)]:
matches = [(m.group(), m.span()) for m in pattern.finditer(sample)]
parts, last = [], 0
for m in pattern.finditer(sample):
parts += [sample[last:m.start()].lower(), m.group()]
last = m.end()
parts.append(sample[last:].lower())
print(f'{name}: matches={matches}; normalized={repr("".join(parts))}')
print()
PY
printf '%s\n' '--- GoodData MAQL escape references in repository metadata ---'
rg -n -i -C 3 'gooddata.*maql|maql.*(literal|escape|quote)|where .*".*"' \
packages/gooddata-eval README.md pyproject.toml setup.cfg 2>/dev/null | head -250 || trueRepository: gooddata/gooddata-python-sdk Length of output: 7402 🌐 Web query:
💡 Result: In GoodData's Multidimensional Analytical Query Language (MAQL), string literals are enclosed in double quotation marks (e.g., "value") [1][2]. To include a double quote character within a string literal, you must escape it using a backslash (") [3]. Similarly, other special characters or literal backslashes within strings are escaped using the backslash character [3]. For example, when a string value contains an apostrophe, it may be rendered or treated as "O'Brien" [3]. Additionally, in the context of the LIKE clause, which is used for pattern matching, the backslash character is used as an escape character for special pattern-matching symbols: to match a literal underscore () or percent sign (%) without triggering their wildcard behavior, they must be preceded by two backslashes (e.g., "\%" or "\") [4]. Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
current = re.compile(r'\{[^}]*\}|"[^"]*"|\'[^\']*\'')
proposed = re.compile(r'\{[^}]*\}|"(?:\\.|[^"\\])*"|\'(?:\\.|[^\'\\])*\'')
# MAQL text for a literal whose value is the quoted string `"Active"` or `"active"`.
samples = [
'"' + '\\' + '"' + 'Active' + '\\' + '"' + '"',
'"' + '\\' + '"' + 'active' + '\\' + '"' + '"',
]
for name, pattern in [('current', current), ('proposed', proposed)]:
normalized = []
for sample in samples:
parts, last = [], 0
for match in pattern.finditer(sample):
parts += [sample[last:match.start()].lower(), match.group()]
last = match.end()
parts.append(sample[last:].lower())
normalized.append(''.join(parts))
print(name)
print(' inputs: ', [repr(s) for s in samples])
print(' normalized: ', [repr(s) for s in normalized])
print(' equal: ', normalized[0] == normalized[1])
PYRepository: gooddata/gooddata-python-sdk Length of output: 496 Handle escaped quotes in protected MAQL literals. The current pattern normalizes escaped 🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| 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]: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note keywords do not have to be all uppercase. You can create a metric with lowercase keywords.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, and actually consistent with the fix here rather than a gap in it:
_casefold_outside_protectedlowercases every MAQL keyword/operator regardless of what case the agent (or a user) originally wrote it in, specifically so that a lowercase-keyword MAQL and an uppercase-keyword MAQL normalize to the same string and compare equal. So a metric created with lowercase keywords is exactly the case this PR makes work correctly — no change needed here.