aminghadersohi commented on code in PR #43771:
URL: https://github.com/apache/superset/pull/43771#discussion_r4188365145
##########
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` lists metric slots by
field name, and `SunburstChartConfig.secondary_metric` is not in the list. A
sunburst `secondary_metric` that is a near miss of a saved metric (with no
`saved_metric: true`) therefore still ends with "No matching columns found" and
no saved-metric hint, while `metric` on the same config does get the hint.
Consider adding `"secondary_metric"` to the field tuple in
`_extract_metric_references`, ideally with a sunburst test.
##########
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"
column = f.get("subject") or f.get("col")
if not column or not isinstance(column, str):
continue
- clause = f.get("clause", "WHERE").upper()
try:
if not _adhoc_filter_column_valid(column, clause, dataset_context):
invalid.append((column, clause))
except AmbiguousDatasetReferenceError as ex:
return DatasetValidator._build_ambiguous_reference_error(ex)
if not invalid:
+ if has_simple_having:
Review Comment:
**P3:** Because the HAVING rejection is deferred until after column
validation, the invalid-column path below still runs first and passes
`include_metrics=True` for HAVING (from master's `_get_column_suggestions`
call). So a HAVING subject that is a near miss of a saved metric gets told to
use the metric. Probe: `{"clause": "HAVING", "subject": "sum_boyz"}` on the
`_orm_dataset()` fixture returns `CHART_VALIDATION_FAILED` with suggestions
`['sum_boys']`. After following that suggestion, the caller gets
`UNSUPPORTED_FILTER_CLAUSE`, so the guidance is a dead end. Suggest checking
`has_simple_having` before the invalid-column error, or leaving metrics out of
the suggestions while HAVING is unsupported.
--
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]
