sadpandajoe commented on code in PR #45904:
URL: https://github.com/apache/superset/pull/45904#discussion_r4447658779


##########
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 empty-input COUNT/COUNT_DISTINCT branch here never runs against the 
bundled fixture: the example provider's only metric is `SUM(amount)`, so 
`metric.aggregation in _COUNTS` is never true. Deleting this branch, or a 
provider that returns NULL instead of 0 for an ungrouped empty IN COUNT, would 
leave every mutation test in `test_conformance.py` passing. Could the example 
provider gain a COUNT metric, with a mutation test returning NULL for it that 
must raise `AssertionError`?



-- 
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]

  • [PR] fix(se... mikebridge (via GitHub) via notifications
    • Re: [P... netlify[bot] (via GitHub) via notifications
    • Re: [P... codecov[bot] (via GitHub) via notifications
    • Re: [P... mikebridge (via GitHub) via notifications
    • Re: [P... sadpandajoe (via GitHub) via notifications
    • Re: [P... mikebridge (via GitHub) via notifications
    • Re: [P... sadpandajoe (via GitHub) via notifications
    • Re: [P... sadpandajoe (via GitHub) via notifications
    • Re: [P... codeant-ai-for-open-source[bot] (via GitHub) via notifications
    • Re: [P... mikebridge (via GitHub) via notifications
    • Re: [P... mikebridge (via GitHub) via notifications

Reply via email to