aminghadersohi commented on code in PR #43771:
URL: https://github.com/apache/superset/pull/43771#discussion_r4188278269
##########
superset/mcp_service/chart/compile.py:
##########
@@ -237,23 +281,47 @@ def _validate_adhoc_filter_columns(
# (column, clause) pairs: the clause decides whether a saved metric is a
# legal reference, and so whether metrics belong in the suggestions.
invalid: list[tuple[str, str]] = []
+ has_simple_having = False
for f in adhoc_filters:
# SIMPLE filters expose the column via "subject"; SQL-expression
# filters carry a free-form ``sqlExpression`` we can't safely parse,
# so skip those.
if f.get("expressionType") and f.get("expressionType") != "SIMPLE":
continue
+ clause = f.get("clause", "WHERE")
+ if not isinstance(clause, str) or clause not in {"WHERE", "HAVING"}:
+ return ChartGenerationError(
+ error_type="invalid_filter_clause",
+ message="SIMPLE filter clause must be 'WHERE' or 'HAVING'",
+ details=(
+ f"A SIMPLE filter has malformed clause {clause!r}; the
clause "
+ "is never coerced or defaulted when explicitly set."
+ ),
+ suggestions=["Use clause='WHERE'"],
+ error_code="INVALID_FILTER_CLAUSE",
+ )
+ has_simple_having = has_simple_having or clause == "HAVING"
Review Comment:
P3: Deferring the SIMPLE HAVING rejection until after the column check keeps
master's `test_metric_suggestions_follow_the_filter_clause` green, but on this
branch its premise no longer holds. A HAVING filter with a near-miss subject
(e.g. `sum_boy`) gets `invalid_column` with `sum_boys` suggested
(`include_metrics=clause == "HAVING"`). The caller applies that fix and then
hits `UNSUPPORTED_FILTER_CLAUSE`, so the suggestion leads to a dead end.
Suggest returning the unsupported-clause error first whenever any SIMPLE HAVING
filter is present (that is still order-independent), dropping the HAVING metric
suggestions, and updating that master test to expect
`UNSUPPORTED_FILTER_CLAUSE` for HAVING.
##########
superset/mcp_service/chart/validation/dataset_validator.py:
##########
@@ -252,13 +332,43 @@ def validate_against_dataset(
# handlebars / big number slip through ``SUM(non_numeric)`` patterns
# for the fast-path tools that skip Tier 2.
aggregation_errors = DatasetValidator._validate_aggregations(
- column_refs, dataset_context
+ column_refs,
+ dataset_context,
+ require_numeric_metrics=isinstance(config, SunburstChartConfig),
Review Comment:
P3: Master's new `_extract_metric_references` (field tuple `("y",
"y_secondary", "metric", "metrics", "size")`) does not include Sunburst's
`secondary_metric`. A near-miss saved-metric name there falls through to "No
matching columns found" with no saved-metric hint, while the same typo in
`metric` gets one. Adding `"secondary_metric"` to that tuple, plus a small
test, would bring Sunburst into the merged hinting behaviour.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
