sadpandajoe commented on code in PR #45904:
URL: https://github.com/apache/superset/pull/45904#discussion_r4444427868
##########
UPDATING.md:
##########
@@ -24,6 +24,17 @@ assists people when migrating to a new version.
## Next
+- Semantic-view filters with an empty IN or NOT IN selection no longer fail
with a
Review Comment:
This new entry says empty IN/NOT IN selections are no longer refused, but
the "MCP get_chart_data error categories" section further down (around line
592) still gives "a semantic view refusing an empty `IN` filter" as its example
of a `ValidationError`. An MCP client author following that example would
expect a validation error for an empty semantic IN, and would instead get a
successful empty or aggregate result. Should that example switch to the missing
comparison operand case the updated MCP tests now use?
##########
superset-core/src/superset_core/semantic_layers/testing.py:
##########
@@ -154,6 +158,100 @@ def test_query_outputs(
d.name for d in query.dimensions
} | {m.name for m in query.metrics}
+ @pytest.mark.parametrize("operator", [Operator.IN, Operator.NOT_IN])
+ def test_empty_membership(
+ self,
+ operator: Operator,
+ semantic_view: SemanticView,
+ semantic_query_cases: Mapping[str, tuple[SemanticQuery, pa.Table]],
+ ) -> None:
+ """Empty IN is FALSE and empty NOT IN is TRUE where the filter sits.
+
+ Derived from the ``identity`` case. The filtered member is the one
with a
+ NULL group, so an empty NOT IN that drops NULL rows fails.
+ """
+ query: SemanticQuery
+ expected: pa.Table
+ query, expected = semantic_query_cases["identity"]
+ dimension: Dimension | None = next(
+ (d for d in query.dimensions if
expected.column(d.name).null_count),
+ None,
+ )
+ assert dimension is not None, (
+ "identity fixture requires a NULL group to witness empty NOT IN"
+ )
+ empty: Filter = Filter(PredicateType.WHERE, dimension, operator,
frozenset())
+ filters: set[FilterExpression] = set(query.filters or set())
+ grouped: SemanticResult = semantic_view.get_table(
+ replace(query, filters={*filters, empty})
+ )
+ _assert_result_equal(
+ grouped.results,
+ expected if operator == Operator.NOT_IN else expected.slice(0, 0),
+ )
+ if not query.metrics:
+ return
+ # Without grouping, SQL still aggregates zero input rows into one row.
+ # Keep the fixture's other fields, such as its selection identity
version.
+ ungrouped: SemanticQuery = replace(
+ query,
+ dimensions=[],
+ filters={*filters, empty},
+ order=None,
+ limit=None,
+ offset=None,
+ group_limit=None,
+ )
+ actual: pa.Table = semantic_view.get_table(ungrouped).results
+ if operator == Operator.NOT_IN:
+ unfiltered: SemanticQuery = replace(ungrouped, filters=filters or
None)
+ _assert_result_equal(actual,
semantic_view.get_table(unfiltered).results)
+ return
+ assert actual.num_rows == 1, (
+ "an ungrouped aggregate over an empty IN returns one row"
+ )
+ metric: Metric
+ for metric in query.metrics:
+ value: object = actual.column(metric.name)[0].as_py()
+ if metric.aggregation in _COUNTS:
Review Comment:
The only metric in the SDK example provider is `SUM(amount)`, but
`conformance_provider.py` builds it without an `aggregation`, so
`metric.aggregation` is `None` here and neither the `_COUNTS` nor the
`_NULL_WHEN_EMPTY` branch ever runs. A provider that returns one row containing
`0` instead of NULL for an ungrouped empty IN would still pass
`test_empty_membership`, and the mutation self-tests would not catch it either.
Could the example metric declare `AggregationType.SUM`, with a mutation case
that returns a wrong scalar and must fail?
--
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]
