From 4f8d219047e6c2d352cc9c8d7022e942631ed71b Mon Sep 17 00:00:00 2001 From: Mike Bridge Date: Fri, 30 Oct 2026 10:32:41 -0600 Subject: [PATCH 1/3] feat(semantic-layer): sort NULL dates last in the default order An unordered semantic query is sorted by its first temporal dimension ascending, but the SDK left NULL placement to the provider while the cache rollup put NULL values last. With a limit, a provider that sorts NULL first and a cached rollup could return different rows. The SDK order term can now carry an optional NULL placement: OrderTerm(member, direction, nulls) still unpacks as a (member, direction) pair, so existing providers keep working, and null_placement(term) reads it from either form. The host sends its generated default as ascending with NULL values last, which also tells it apart from an explicit ascending order on the same dimension. Explicit orders are unchanged. The cache identity includes the placement and moves to v6, so cached semantic results are recomputed once. The conformance kit adds a required null_placement case. --- UPDATING.md | 13 +++ .../semantic-layer-provider-testing.md | 19 +++- .../superset_core/semantic_layers/testing.py | 68 +++++++++++- .../superset_core/semantic_layers/types.py | 101 +++++++++++++++-- .../src/superset_core/semantic_layers/view.py | 5 +- .../semantic_layers/conformance_provider.py | 13 ++- .../tests/semantic_layers/test_conformance.py | 74 +++++++++++++ .../tests/semantic_layers/test_types.py | 89 ++++++++++++++- superset/semantic_layers/cache_identity.py | 38 ++++--- superset/semantic_layers/cache_transform.py | 10 +- superset/semantic_layers/ordering.py | 14 ++- .../semantic_layers/cache_host_test.py | 4 +- .../semantic_layers/cache_identity_test.py | 33 +++++- .../semantic_layers/cache_or_filters_test.py | 2 +- .../semantic_layers/cache_repository_test.py | 2 +- .../semantic_layers/default_order_test.py | 102 +++++++++++++++++- .../unit_tests/semantic_layers/mapper_test.py | 14 ++- 17 files changed, 540 insertions(+), 61 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 3e564449ed8d..f511b42a3ea7 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -75,6 +75,19 @@ assists people when migrating to a new version. up. Clients adding more views should split them across requests and retry timed-out views. +- **Semantic-layer default order puts NULL dates last.** When a semantic query + has no explicit order, Superset sorts by its first date, timestamp or time + dimension ascending with NULL values last, and sends that order to the + provider as `OrderTerm(dimension, OrderDirection.ASC, NullPlacement.LAST)`. + `OrderTerm` still unpacks as a `(member, direction)` pair, so existing + providers keep running; to return the same rows as Superset's cache, a + provider must apply the placement (read it with `null_placement(term)`) before + `offset` and `limit`, and keep it when it rebuilds order terms. Explicit orders + are sent unchanged and keep the provider's own NULL placement. The provider + conformance kit adds a required `null_placement` query case. The semantic + result cache key format changes, so cached semantic results are recomputed + once after upgrading. + - `superset-frontend` type-checks with TypeScript 7, whose native compiler no longer ships the JavaScript compiler API. The webpack dev server therefore no longer runs `fork-ts-checker-webpack-plugin`, and the `DISABLE_TS_CHECKER` diff --git a/docs/developer_docs/semantic-layer-provider-testing.md b/docs/developer_docs/semantic-layer-provider-testing.md index 6484b8a1ba8a..3e68525c04ef 100644 --- a/docs/developer_docs/semantic-layer-provider-testing.md +++ b/docs/developer_docs/semantic-layer-provider-testing.md @@ -59,8 +59,16 @@ separate integration tests. | `semantic_change_catalog` | No-argument callable changing the controlled source catalog: add a visible member on every call, without clearing provider caches. | | `semantic_provider_class` | `(importable_module_name, class_name)` of the layer for an isolated import and static-schema check. Dependencies must already be installed. | -Required query cases are `identity`, `grain`, `null`, `not_null`, `or`, and -`order_limit`. Use member names distinct from private IDs and display labels. +Required query cases are `identity`, `grain`, `null`, `not_null`, `or`, +`order_limit` and `null_placement`. Use member names distinct from private IDs +and display labels. +For `null_placement`, order first by a selected dimension with an +`OrderTerm(member, direction, NullPlacement.LAST)` (or `FIRST`) and a limit, and +seed NULL and non-NULL values of that member so the limited page keeps different +rows under the opposite placement. The SQLite example sorts `category` ascending +with NULL values last and a limit of 2, keeping A and B where SQLite's own +default would keep NULL and A. The suite also checks that the unlimited result +puts every NULL value of that member at the requested end. For `grain`, use a grain-bearing dimension with data whose aggregation distinguishes the selected grain from raw values. Multiple grain variants for one name are allowed, not required. When the catalog declares no grain-bearing dimension, the case skips with an explicit reason before looking up its fixture entry; such providers may omit the `grain` entry. This reports unexercised grain coverage, not a passing grain check. NULL and grouped-OR fixtures must select NULL and non-NULL branches observably; the `or` case must use the host-normalized shape: one `OrFilter` containing @@ -108,9 +116,10 @@ unverified release-gate coverage, not passing conformance; do not fabricate an SDK exception just to fill the matrix. The count lane only checks failure propagation; it does not settle pending count or value-shape rules. Construction fixtures avoid copying a factory signature or context type. -The host supplies default chronological ordering before calling the provider. -This suite checks the provider obligation to honor explicit ordering before limit -and offset, including the ordered grain case. +The host supplies default chronological ordering before calling the provider, +as an ascending `OrderTerm` with NULL values last. This suite checks the provider +obligation to honor explicit ordering and NULL placement before limit and offset, +including the ordered grain case. The import/schema check starts a fresh Python interpreter with the test import path. It rejects imports of `superset`, `flask` and `flask_appbuilder`, including diff --git a/superset-core/src/superset_core/semantic_layers/testing.py b/superset-core/src/superset_core/semantic_layers/testing.py index 879c49ac3a4b..31d7b1f048b6 100644 --- a/superset-core/src/superset_core/semantic_layers/testing.py +++ b/superset-core/src/superset_core/semantic_layers/testing.py @@ -42,6 +42,8 @@ Dimension, Filter, Metric, + null_placement, + NullPlacement, Operator, OrFilter, SemanticQuery, @@ -87,7 +89,8 @@ class SemanticProviderConformance: """Subclass in a ``Test...`` class and supply the documented provider fixtures. Query fixtures seed discriminating data and provide independent expected - Arrow tables for identity, grain, NULL, grouped OR, and order-before-limit. + Arrow tables for identity, grain, NULL, grouped OR, order-before-limit and + NULL placement. Error fixtures call real provider methods against controlled backend outcomes. See the provider author guide for every fixture and the limits of this suite. """ @@ -101,7 +104,16 @@ def test_catalog_identity( assert _catalog(semantic_view) == _catalog(semantic_reloaded_view) @pytest.mark.parametrize( - "case", ["identity", "grain", "null", "not_null", "or", "order_limit"] + "case", + [ + "identity", + "grain", + "null", + "not_null", + "or", + "order_limit", + "null_placement", + ], ) def test_query_outputs( self, @@ -148,6 +160,8 @@ def test_query_outputs( _assert_result_equal( full.results.slice(query.offset or 0, query.limit), expected ) + elif case == "null_placement": + _assert_null_placement_witness(query, semantic_view, expected) result: SemanticResult = semantic_view.get_table(query) _assert_result_equal(result.results, expected) assert set(result.results.column_names) == { @@ -272,6 +286,56 @@ def test_imports_and_static_schema( assert completed.returncode == 0, completed.stdout + completed.stderr +def _assert_null_placement_witness( + query: SemanticQuery, view: SemanticView, expected: pa.Table +) -> None: + """Require NULL values of the first order key where its placement puts them. + + The host's default chronological order asks for NULL values last; a + provider must honor a placement on any order key, before offset and limit. + The page must keep different rows from the ones the opposite placement would. + """ + assert query.order, "null_placement fixture requires an order" + first: tuple[object, ...] = query.order[0] + placement: NullPlacement | None = null_placement(first) + assert placement is not None, ( + "null_placement fixture's first order key requires a NullPlacement" + ) + member: object = first[0] + assert isinstance(member, Dimension), ( + "null_placement fixture must order by a selected dimension" + ) + assert member in query.dimensions, ( + "null_placement fixture must order by a selected dimension" + ) + assert query.limit is not None + assert query.limit > 0 + full: pa.Table = view.get_table(replace(query, limit=None, offset=None)).results + values: list[object] = full.column(member.name).to_pylist() + nulls: list[object] = [value for value in values if value is None] + others: list[object] = [value for value in values if value is not None] + assert nulls, "null_placement fixture requires NULL values of the order key" + assert others, "null_placement fixture requires non-NULL values of the order key" + placed: list[object] = ( + others + nulls if placement is NullPlacement.LAST else nulls + others + ) + opposite: list[object] = ( + nulls + others if placement is NullPlacement.LAST else others + nulls + ) + assert values == placed, ( + f"{member.name}: NULL values must sort {placement.value.lower()}, got {values}" + ) + start: int = query.offset or 0 + stop: int = start + query.limit + assert sorted(map(repr, placed[start:stop])) != sorted( + map(repr, opposite[start:stop]) + ), ( + "null_placement fixture's page must keep different rows " + "under the opposite placement" + ) + _assert_result_equal(full.slice(start, query.limit), expected) + + def _assert_normalized_or_witness(query: SemanticQuery) -> None: """Require the host's mixed-IN/NULL group and an outer comparison.""" groups: list[OrFilter] = [ diff --git a/superset-core/src/superset_core/semantic_layers/types.py b/superset-core/src/superset_core/semantic_layers/types.py index fe5a85ee6faf..1e831d09c0ca 100644 --- a/superset-core/src/superset_core/semantic_layers/types.py +++ b/superset-core/src/superset_core/semantic_layers/types.py @@ -315,9 +315,85 @@ class OrderDirection(enum.Enum): DESC = "DESC" +class NullPlacement(enum.Enum): + """Where NULL values of an order key sort, relative to all other values.""" + + FIRST = "FIRST" + LAST = "LAST" + + OrderTuple = tuple[Metric | Dimension | AdhocExpression, OrderDirection] +class OrderTerm(tuple[Metric | Dimension | AdhocExpression, OrderDirection]): + """ + An ``(member, direction)`` order pair with an optional NULL placement. + + It unpacks, indexes and has the length of a two-element ``OrderTuple``, so a + provider reading ``member, direction`` keeps working. Read the placement with + :func:`null_placement`, which also accepts a plain pair. + + ``nulls`` of ``None`` means the provider's default placement, the same as a + plain pair, and such a term equals the plain pair. A term with a placement + differs from the pair without one: the provider must sort NULL values of + that key first or last, before ``offset`` and ``limit``. + """ + + nulls: NullPlacement | None + + def __new__( + cls, + member: Metric | Dimension | AdhocExpression, + direction: OrderDirection, + nulls: NullPlacement | None = None, + ) -> OrderTerm: + if not isinstance(direction, OrderDirection): + raise TypeError("OrderTerm direction must be an OrderDirection") + if nulls is not None and not isinstance(nulls, NullPlacement): + raise TypeError("OrderTerm nulls must be a NullPlacement or None") + term: OrderTerm = super().__new__(cls, (member, direction)) + object.__setattr__(term, "nulls", nulls) + return term + + def __setattr__(self, name: str, value: object) -> None: + raise AttributeError("OrderTerm is immutable") + + def __delattr__(self, name: str) -> None: + raise AttributeError("OrderTerm is immutable") + + def __getnewargs__( + self, + ) -> tuple[ + Metric | Dimension | AdhocExpression, OrderDirection, NullPlacement | None + ]: + return (self[0], self[1], self.nulls) + + def __eq__(self, other: object) -> bool: + if not isinstance(other, tuple): + return NotImplemented + return tuple.__eq__(self, other) is True and self.nulls == null_placement(other) + + def __ne__(self, other: object) -> bool: + equal: bool = self.__eq__(other) + return equal if equal is NotImplemented else not equal + + def __hash__(self) -> int: + if self.nulls is None: + return tuple.__hash__(self) + return hash((tuple(self), self.nulls)) + + def __repr__(self) -> str: + return f"OrderTerm({self[0]!r}, {self[1]!r}, nulls={self.nulls!r})" + + +def null_placement(term: tuple[object, ...]) -> NullPlacement | None: + """Return an order term's NULL placement, or ``None`` for the provider default. + + Accepts a plain ``(member, direction)`` pair, which has no placement. + """ + return getattr(term, "nulls", None) + + @dataclass(frozen=True) class GroupLimit: """ @@ -374,20 +450,25 @@ class SemanticQuery: Truncating first and sorting the truncated page returns a different, and wrong, answer. ``order`` entries are ``(member, direction)`` pairs whose member must be defined by the view; an adhoc expression is only a valid - order key for a view declaring ``ADHOC_EXPRESSIONS_IN_ORDERBY``. + order key for a view declaring ``ADHOC_EXPRESSIONS_IN_ORDERBY``. An entry + may be an :class:`OrderTerm` carrying a :class:`NullPlacement`; the + provider must then sort that key's NULL values first or last before + ``offset`` and ``limit``. A plain pair, or a term without a placement, + leaves NULL placement to the provider. **Default chronological order.** When ``order`` is empty (``None`` or ``[]``) and ``dimensions`` includes a date, timestamp or time dimension, the host sorts ascending by the first such dimension in ``dimensions`` (by list - position, not ``SemanticView.preferred_temporal_dimension``), before - ``offset`` and ``limit``. Rows that tie on it come back in no particular - order, and where its NULL values sort is not specified. The host applies - this both to the ``get_table`` query it sends the provider and to results it - serves from its cache. It is a default, never a prefix: a non-empty - ``order`` is used exactly as given. A ``get_table`` call therefore receives - an empty ``order`` only when no temporal dimension is selected, and may then - return rows in any order. ``get_row_count`` gets no default, since order - cannot change a count. + position, not ``SemanticView.preferred_temporal_dimension``), with its NULL + values last, before ``offset`` and ``limit``. Rows that tie on it come back + in no particular order. The host applies this both to the ``get_table`` + query it sends the provider, as + ``OrderTerm(dimension, OrderDirection.ASC, NullPlacement.LAST)``, and to + results it serves from its cache. It is a default, never a prefix: a + non-empty ``order`` is used exactly as given. A ``get_table`` call therefore + receives an empty ``order`` only when no temporal dimension is selected, and + may then return rows in any order. ``get_row_count`` gets no default, since + order cannot change a count. ``group_limit`` is a separate ranking, with its own metric, direction and optional filters. diff --git a/superset-core/src/superset_core/semantic_layers/view.py b/superset-core/src/superset_core/semantic_layers/view.py index 0b84a67b4686..55be652120ef 100644 --- a/superset-core/src/superset_core/semantic_layers/view.py +++ b/superset-core/src/superset_core/semantic_layers/view.py @@ -172,8 +172,9 @@ def get_table(self, query: SemanticQuery) -> SemanticResult: Raise SemanticQueryRejectedError for deliberate input rejection. Leave operational and unexpected failures unclassified. - Sort by ``query.order`` before applying ``query.offset`` and - ``query.limit``; truncating before sorting returns the wrong rows. Name + Sort by ``query.order``, including any term's NULL placement, before + applying ``query.offset`` and ``query.limit``; truncating before sorting + returns the wrong rows. Name each result column after the member that produced it. Raise ``superset_core.semantic_layers.errors.SemanticResultCompletenessError`` diff --git a/superset-core/tests/semantic_layers/conformance_provider.py b/superset-core/tests/semantic_layers/conformance_provider.py index 08290dd959d5..d370216a1517 100644 --- a/superset-core/tests/semantic_layers/conformance_provider.py +++ b/superset-core/tests/semantic_layers/conformance_provider.py @@ -37,6 +37,8 @@ FilterExpression, Grains, Metric, + null_placement, + NullPlacement, Operator, OrFilter, SemanticQuery, @@ -151,9 +153,9 @@ def get_table(self, query: SemanticQuery) -> SemanticResult: if any(member not in columns for member, _ in query.order): raise SemanticQueryRejectedError(SemanticQueryErrorCode.INVALID_QUERY) sql += " ORDER BY " + ", ".join( - f'"{member.name}" {direction.value}' - for member, direction in query.order - if not isinstance(member, AdhocExpression) + f'"{term[0].name}" {term[1].value}{_nulls(null_placement(term))}' + for term in query.order + if not isinstance(term[0], AdhocExpression) ) if query.limit is not None: sql += " LIMIT ? OFFSET ?" @@ -181,6 +183,11 @@ def get_row_count(self, query: SemanticQuery) -> SemanticResult: raise SemanticQueryRejectedError(SemanticQueryErrorCode.UNSUPPORTED_QUERY) +def _nulls(placement: NullPlacement | None) -> str: + """Render a requested NULL placement; without one, SQLite's default applies.""" + return f" NULLS {placement.value}" if placement is not None else "" + + def _predicate(expression: FilterExpression, parameters: list[object]) -> str: """Render only the normalized predicates used by this example.""" if isinstance(expression, OrFilter): diff --git a/superset-core/tests/semantic_layers/test_conformance.py b/superset-core/tests/semantic_layers/test_conformance.py index 04b4b01efcf6..41374fd1f65e 100644 --- a/superset-core/tests/semantic_layers/test_conformance.py +++ b/superset-core/tests/semantic_layers/test_conformance.py @@ -32,8 +32,10 @@ FilterExpression, Grains, Metric, + NullPlacement, Operator, OrderDirection, + OrderTerm, OrFilter, PredicateType, SemanticQuery, @@ -151,6 +153,16 @@ def semantic_query_cases( ), pa.table({"category": ["B"], "total": [8]}), ), + # SQLite sorts NULL first for ASC: ignoring the placement keeps NULL, A. + "null_placement": ( + SemanticQuery( + [total], + [category], + order=[OrderTerm(category, OrderDirection.ASC, NullPlacement.LAST)], + limit=2, + ), + pa.table({"category": ["A", "B"], "total": [7, 8]}), + ), } @@ -646,3 +658,65 @@ def must_not_execute(query: SemanticQuery) -> SemanticResult: SemanticProviderConformance().test_query_outputs( "or", semantic_view, {"or": (query, expected)} ) + + +@pytest.mark.parametrize( + ("mutation", "diagnostic"), + [ + ("plain_pair", "requires a NullPlacement"), + ("whole_result", "must keep different rows"), + ("no_nulls", "requires NULL values"), + ], +) +def test_null_placement_lane_rejects_weak_fixtures( + semantic_view: "QueryView", + semantic_query_cases: dict[str, tuple[SemanticQuery, pa.Table]], + mutation: str, + diagnostic: str, +) -> None: + """A witness must request a placement and keep a page the other one would not.""" + query: SemanticQuery + expected: pa.Table + query, expected = semantic_query_cases["null_placement"] + category: Dimension = query.dimensions[0] + if mutation == "plain_pair": + query = replace(query, order=[(category, OrderDirection.ASC)]) + elif mutation == "whole_result": + # A limit covering every row keeps the same rows under either placement. + query = replace(query, limit=4) + expected = pa.table( + { + "category": pa.array(["A", "B", "C", None], type=pa.string()), + "total": [7, 8, 6, 4], + } + ) + else: + query = replace( + query, + filters={Filter(PredicateType.WHERE, category, Operator.IS_NOT_NULL, None)}, + ) + + with pytest.raises(AssertionError, match=diagnostic): + SemanticProviderConformance().test_query_outputs( + "null_placement", semantic_view, {"null_placement": (query, expected)} + ) + + +def test_null_placement_lane_rejects_a_provider_ignoring_the_placement( + semantic_view: "QueryView", + semantic_query_cases: dict[str, tuple[SemanticQuery, pa.Table]], + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A provider that drops the placement sorts NULL by its engine default.""" + original: Callable[[SemanticQuery], SemanticResult] = semantic_view.get_table + + def drop_placement(query: SemanticQuery) -> SemanticResult: + return original( + replace(query, order=[(term[0], term[1]) for term in query.order or []]) + ) + + monkeypatch.setattr(semantic_view, "get_table", drop_placement) + with pytest.raises(AssertionError, match="NULL values must sort last"): + SemanticProviderConformance().test_query_outputs( + "null_placement", semantic_view, semantic_query_cases + ) diff --git a/superset-core/tests/semantic_layers/test_types.py b/superset-core/tests/semantic_layers/test_types.py index 15e74c862bf3..ddc52c0d863d 100644 --- a/superset-core/tests/semantic_layers/test_types.py +++ b/superset-core/tests/semantic_layers/test_types.py @@ -15,8 +15,21 @@ # specific language governing permissions and limitations # under the License. +import copy +import pickle +from dataclasses import replace + import pyarrow as pa -from superset_core.semantic_layers.types import Dimension, Metric +import pytest +from superset_core.semantic_layers.types import ( + Dimension, + Metric, + null_placement, + NullPlacement, + OrderDirection, + OrderTerm, + SemanticQuery, +) def test_dimension_metadata_is_not_part_of_identity() -> None: @@ -95,3 +108,77 @@ def test_metadata_defaults_are_not_shared() -> None: first.metadata["display_name"] = "First" assert second.metadata == {} + + +ORDER_DATE = Dimension("orders.order_date", "order_date", pa.date32()) + + +def test_order_term_reads_like_a_plain_pair() -> None: + term = OrderTerm(ORDER_DATE, OrderDirection.ASC, NullPlacement.LAST) + + member, direction = term + + assert (member, direction) == (ORDER_DATE, OrderDirection.ASC) + assert len(term) == 2 + assert term[0] is ORDER_DATE + assert isinstance(term, tuple) + assert null_placement(term) is NullPlacement.LAST + assert null_placement((ORDER_DATE, OrderDirection.ASC)) is None + + +def test_order_term_without_placement_equals_the_plain_pair() -> None: + pair = (ORDER_DATE, OrderDirection.ASC) + term = OrderTerm(ORDER_DATE, OrderDirection.ASC) + + assert term == pair + assert pair == term + assert hash(term) == hash(pair) + assert {term, pair} == {pair} + + +@pytest.mark.parametrize("nulls", list(NullPlacement)) +def test_order_term_placement_distinguishes_it_from_the_plain_pair( + nulls: NullPlacement, +) -> None: + pair = (ORDER_DATE, OrderDirection.ASC) + term = OrderTerm(ORDER_DATE, OrderDirection.ASC, nulls) + + assert term != pair + assert pair != term + assert not term == pair # noqa: SIM201 + assert term != OrderTerm(ORDER_DATE, OrderDirection.ASC) + assert term == OrderTerm(ORDER_DATE, OrderDirection.ASC, nulls) + assert len({term, pair, OrderTerm(ORDER_DATE, OrderDirection.ASC, nulls)}) == 2 + assert SemanticQuery([], [ORDER_DATE], order=[term]) != SemanticQuery( + [], [ORDER_DATE], order=[pair] + ) + + +def test_order_term_survives_copy_and_pickle() -> None: + term = OrderTerm(ORDER_DATE, OrderDirection.DESC, NullPlacement.FIRST) + query = replace(SemanticQuery([], [ORDER_DATE], order=[term])) + + for restored in ( + copy.copy(term), + copy.deepcopy(term), + pickle.loads(pickle.dumps(term)), # noqa: S301 + *(query.order or []), + ): + assert type(restored) is OrderTerm + assert restored == term + assert null_placement(restored) is NullPlacement.FIRST + + +def test_order_term_is_immutable_and_validated() -> None: + term = OrderTerm(ORDER_DATE, OrderDirection.ASC, NullPlacement.LAST) + + with pytest.raises(AttributeError): + term.nulls = None + with pytest.raises(TypeError): + OrderTerm(ORDER_DATE, "ASC") # type: ignore[arg-type] + with pytest.raises(TypeError): + OrderTerm(ORDER_DATE, OrderDirection.ASC, "LAST") # type: ignore[arg-type] + assert repr(term) == ( + f"OrderTerm({ORDER_DATE!r}, {OrderDirection.ASC!r}, " + f"nulls={NullPlacement.LAST!r})" + ) diff --git a/superset/semantic_layers/cache_identity.py b/superset/semantic_layers/cache_identity.py index c982f91d99e3..0bbdc9238c23 100644 --- a/superset/semantic_layers/cache_identity.py +++ b/superset/semantic_layers/cache_identity.py @@ -32,6 +32,8 @@ FilterExpression, GroupLimit, Metric, + null_placement, + NullPlacement, OrderTuple, OrFilter, SemanticQuery, @@ -41,7 +43,7 @@ # Prefixes every bucket and value key. Bump it whenever the pickled shape of # ``cache_types.CachedEntry`` or the canonical query material changes. -IDENTITY_FORMAT_VERSION: str = "v5" +IDENTITY_FORMAT_VERSION: str = "v6" _SENSITIVE_KEY_PARTS: tuple[str, ...] = ( "credential", "password", @@ -161,15 +163,7 @@ def order(order: list[OrderTuple] | None) -> str: """Serialize ordering for descriptor compatibility checks.""" if not order: return "" - return _canonical_json( - [ - { - "element": _canonical_orderable(element), - "direction": direction.value, - } - for element, direction in order - ] - ) + return _canonical_json(_canonical_order(order)) @staticmethod def filters(filters: set[FilterExpression] | None) -> str: @@ -209,19 +203,29 @@ def _canonical_query(query: SemanticQuery) -> Mapping[str, object]: _canonical_dimension(dimension) for dimension in query.dimensions ], "filters": _canonical_filters(query.filters), - "order": [ - { - "element": _canonical_orderable(element), - "direction": direction.value, - } - for element, direction in query.order or [] - ], + "order": _canonical_order(query.order or []), "limit": query.limit, "offset": query.offset or 0, "group_limit": _canonical_group_limit(query.group_limit), } +def _canonical_order(order: list[OrderTuple]) -> list[Mapping[str, object]]: + """Serialize order terms, including NULL placement when a term sets one.""" + canonical: list[Mapping[str, object]] = [] + for term in order: + element, direction = term + nulls: NullPlacement | None = null_placement(term) + canonical.append( + { + "element": _canonical_orderable(element), + "direction": direction.value, + "nulls": nulls.value if nulls is not None else None, + } + ) + return canonical + + def _canonical_metric(metric: Metric) -> Mapping[str, object]: return { "id": metric.id, diff --git a/superset/semantic_layers/cache_transform.py b/superset/semantic_layers/cache_transform.py index 6e2b6932c317..cfbfe66085da 100644 --- a/superset/semantic_layers/cache_transform.py +++ b/superset/semantic_layers/cache_transform.py @@ -202,14 +202,16 @@ def _rollup(frame: pd.DataFrame, query: SemanticQuery) -> pd.DataFrame: def _sort_by_default_order(frame: pd.DataFrame, query: SemanticQuery) -> pd.DataFrame: """Restore the default order a rollup's grouping replaced, before the limit. - NULL values of the default column sort last. The SDK leaves NULL placement - unspecified, so on an engine that sorts NULLs first a limited rollup and a - provider miss may keep different rows. + NULL values of the default column sort last, as the default order the + provider receives requires, so a limited rollup and a provider miss keep the + same rows. """ dimension: Dimension | None = default_order_dimension(query) if dimension is None: return frame - return frame.sort_values(dimension.name, kind="stable", ignore_index=True) + return frame.sort_values( + dimension.name, kind="stable", na_position="last", ignore_index=True + ) def _project(frame: pd.DataFrame, query: SemanticQuery) -> pd.DataFrame: diff --git a/superset/semantic_layers/ordering.py b/superset/semantic_layers/ordering.py index d5ff42883865..ae5b630b959a 100644 --- a/superset/semantic_layers/ordering.py +++ b/superset/semantic_layers/ordering.py @@ -22,7 +22,9 @@ import pyarrow as pa from superset_core.semantic_layers.types import ( Dimension, + NullPlacement, OrderDirection, + OrderTerm, SemanticQuery, ) @@ -50,10 +52,16 @@ def default_order_dimension(query: SemanticQuery) -> Dimension | None: def with_default_order(query: SemanticQuery) -> SemanticQuery: """Return *query* with its default chronological order filled in. - The provider receives this order on dispatch, and the semantic cache uses - it to decide which cached rows a query may reuse. + The default sorts NULL values last. The term carries that placement, which + also tells it apart from an explicit ascending order on the same dimension, + whose NULL placement is the provider's. The provider receives this order on + dispatch, and the semantic cache uses it to decide which cached rows a query + may reuse. """ dimension: Dimension | None = default_order_dimension(query) if dimension is None: return query - return replace(query, order=[(dimension, OrderDirection.ASC)]) + return replace( + query, + order=[OrderTerm(dimension, OrderDirection.ASC, NullPlacement.LAST)], + ) diff --git a/tests/unit_tests/semantic_layers/cache_host_test.py b/tests/unit_tests/semantic_layers/cache_host_test.py index 183d2cd452d4..f23b3a01a6cf 100644 --- a/tests/unit_tests/semantic_layers/cache_host_test.py +++ b/tests/unit_tests/semantic_layers/cache_host_test.py @@ -369,7 +369,7 @@ def test_cache_identity_does_not_consult_provider_token_or_removed_adapter( def test_definition_digest_rotates_for_grouped_filter_format() -> None: - """Pin the v5 digest, independently computed as SHA256 of its canonical input.""" + """Pin the v6 digest, independently computed as SHA256 of its canonical input.""" assert SemanticCacheIdentityFactory.definition({"changed_on": "None"}).digest == ( - "v5:b947b0b82833f9abfadae97ee05ecfe25a913fa68c0659327b69f1d226b691de" + "v6:1d2f7cac56934582ecc212b52952d3a21b955e57e09c3e2ab849016aadf166cf" ) diff --git a/tests/unit_tests/semantic_layers/cache_identity_test.py b/tests/unit_tests/semantic_layers/cache_identity_test.py index 904e2606cf9d..5e23f1d29945 100644 --- a/tests/unit_tests/semantic_layers/cache_identity_test.py +++ b/tests/unit_tests/semantic_layers/cache_identity_test.py @@ -32,8 +32,11 @@ Grain, GroupLimit, Metric, + NullPlacement, Operator, OrderDirection, + OrderTerm, + OrderTuple, PredicateType, SemanticQuery, ) @@ -58,7 +61,7 @@ def test_identity_is_stable_across_mapping_order() -> None: ).digest assert first == second - assert first.startswith("v5:") + assert first.startswith("v6:") def test_identity_changes_with_result_affecting_material() -> None: @@ -228,6 +231,30 @@ def test_dimension_and_order_identity_cover_all_variants() -> None: assert SemanticCacheIdentityFactory.group_limit(_query().group_limit) +def test_order_identity_includes_null_placement() -> None: + """A placement changes which rows a limit keeps, so it changes both keys.""" + revenue: Metric = _query().metrics[0] + orders: list[list[OrderTuple]] = [ + [(revenue, OrderDirection.DESC)], + [OrderTerm(revenue, OrderDirection.DESC, NullPlacement.LAST)], + [OrderTerm(revenue, OrderDirection.DESC, NullPlacement.FIRST)], + ] + bucket: str = "semantic-cache:bucket:v6:fixture" + + order_keys: set[str] = { + SemanticCacheIdentityFactory.order(order) for order in orders + } + value_keys: set[str] = { + SemanticCacheIdentityFactory.value(bucket, replace(_query(), order=order)) + for order in orders + } + + assert len(order_keys) == len(value_keys) == len(orders) + assert SemanticCacheIdentityFactory.order( + [OrderTerm(revenue, OrderDirection.DESC)] + ) == SemanticCacheIdentityFactory.order([(revenue, OrderDirection.DESC)]) + + def test_bucket_identity_includes_all_host_boundaries() -> None: view: SemanticViewIdentity = SemanticViewIdentity("orders") definition: SemanticDefinitionIdentity = SemanticCacheIdentityFactory.definition( @@ -276,10 +303,10 @@ def test_bucket_identity_includes_all_host_boundaries() -> None: def test_value_identity_combines_bucket_and_query() -> None: - bucket: str = "semantic-cache:bucket:v5:fixture" + bucket: str = "semantic-cache:bucket:v6:fixture" first: str = SemanticCacheIdentityFactory.value(bucket, _query()) second: str = SemanticCacheIdentityFactory.value(bucket, _query()) assert first == second - assert first.startswith("semantic-cache:value:v5:") + assert first.startswith("semantic-cache:value:v6:") diff --git a/tests/unit_tests/semantic_layers/cache_or_filters_test.py b/tests/unit_tests/semantic_layers/cache_or_filters_test.py index d92b790d20ac..d2adb2d5d175 100644 --- a/tests/unit_tests/semantic_layers/cache_or_filters_test.py +++ b/tests/unit_tests/semantic_layers/cache_or_filters_test.py @@ -85,7 +85,7 @@ def test_group_identity_is_order_independent_and_not_flattened() -> None: assert identity != SemanticCacheIdentityFactory.query( replace(query, filters={flattened}) ) - assert ":v5:" in identity + assert ":v6:" in identity @pytest.mark.parametrize("location", ["query", "group_limit", "both"]) diff --git a/tests/unit_tests/semantic_layers/cache_repository_test.py b/tests/unit_tests/semantic_layers/cache_repository_test.py index c42e9ff6aaba..00383f84be54 100644 --- a/tests/unit_tests/semantic_layers/cache_repository_test.py +++ b/tests/unit_tests/semantic_layers/cache_repository_test.py @@ -728,7 +728,7 @@ def test_cached_entry_shape_is_pinned_to_the_identity_version() -> None: "timeout", "write_nonce", ) - assert IDENTITY_FORMAT_VERSION == "v5" + assert IDENTITY_FORMAT_VERSION == "v6" def test_store_surfaces_existence_check_failure_as_store_error() -> None: diff --git a/tests/unit_tests/semantic_layers/default_order_test.py b/tests/unit_tests/semantic_layers/default_order_test.py index 94a85770762d..1f21829c5486 100644 --- a/tests/unit_tests/semantic_layers/default_order_test.py +++ b/tests/unit_tests/semantic_layers/default_order_test.py @@ -29,7 +29,10 @@ AggregationType, Dimension, Metric, + null_placement, + NullPlacement, OrderDirection, + OrderTerm, OrderTuple, SemanticQuery, SemanticResult, @@ -46,6 +49,7 @@ ReuseMode, SemanticCacheLookupResult, ) +from superset.semantic_layers.ordering import with_default_order from tests.unit_tests.semantic_layers.conftest import build_view_meta SALES: Metric = Metric( @@ -83,9 +87,15 @@ def _dispatch(mocker: MockerFixture, query: SemanticQuery) -> SemanticQuery: @pytest.mark.parametrize( "dimensions, expected", [ - # The first selected temporal dimension, ascending. - ([REGION, SHIPPED_AT, ORDER_DATE], [(SHIPPED_AT, OrderDirection.ASC)]), - ([ORDER_DATE, SHIPPED_AT], [(ORDER_DATE, OrderDirection.ASC)]), + # The first selected temporal dimension, ascending, NULL values last. + ( + [REGION, SHIPPED_AT, ORDER_DATE], + [OrderTerm(SHIPPED_AT, OrderDirection.ASC, NullPlacement.LAST)], + ), + ( + [ORDER_DATE, SHIPPED_AT], + [OrderTerm(ORDER_DATE, OrderDirection.ASC, NullPlacement.LAST)], + ), # No temporal dimension: the query stays unordered. ([REGION], None), ([], None), @@ -124,6 +134,32 @@ def test_explicit_order_reaches_the_provider_unchanged( assert _dispatch(mocker, query) is query +def test_default_order_differs_from_an_explicit_ascending_order( + mocker: MockerFixture, +) -> None: + """Only the generated default asks for NULL values last. + + An explicit ascending order on the same dimension keeps the provider's NULL + placement, so the provider must be able to tell the two apart. + """ + unordered: SemanticQuery = SemanticQuery( + metrics=[SALES], dimensions=[ORDER_DATE], limit=2 + ) + explicit: SemanticQuery = replace( + unordered, order=[(ORDER_DATE, OrderDirection.ASC)] + ) + + defaulted: SemanticQuery = _dispatch(mocker, unordered) + + assert defaulted != explicit + assert defaulted == with_default_order(unordered) + assert [null_placement(term) for term in defaulted.order or []] == [ + NullPlacement.LAST + ] + assert _dispatch(mocker, explicit) is explicit + assert [null_placement(term) for term in explicit.order or []] == [None] + + def test_default_order_leaves_the_cache_identity_unchanged( mocker: MockerFixture, ) -> None: @@ -144,7 +180,9 @@ def test_default_order_leaves_the_cache_identity_unchanged( mapper._dispatch_semantic_query(mocker.Mock(), provider, query) assert service.execute.call_args.args[1] is query - assert provider.call_args.args[0].order == [(ORDER_DATE, OrderDirection.ASC)] + assert provider.call_args.args[0].order == [ + OrderTerm(ORDER_DATE, OrderDirection.ASC, NullPlacement.LAST) + ] def test_row_count_dispatch_bypasses_the_cache(mocker: MockerFixture) -> None: @@ -374,3 +412,59 @@ def test_store_replaces_a_descriptor_written_without_the_default_order() -> None assert [candidate.decision.mode for candidate in lookup.candidates] == [ ReuseMode.EXACT ] + + +@pytest.mark.parametrize("stored_explicit", [False, True], ids=["default", "explicit"]) +def test_limited_hit_is_not_shared_between_default_and_explicit_ascending( + stored_explicit: bool, +) -> None: + """A page cut with NULL values last cannot answer one cut by provider default. + + The provider may sort NULL values first for an explicit ascending order, so + the two limited pages can hold different rows. + """ + unordered: SemanticQuery = SemanticQuery( + metrics=[SALES], dimensions=[ORDER_DATE], limit=2 + ) + explicit: SemanticQuery = replace( + unordered, order=[(ORDER_DATE, OrderDirection.ASC)] + ) + rows: dict[str, list[object]] = { + "order_date": [date(2026, 1, 1), date(2026, 2, 1)], + "total_sales": [1.0, 2.0], + } + stored, requested = ( + (explicit, unordered) + if stored_explicit + else ( + unordered, + explicit, + ) + ) + + assert _serve_from_cache(stored, rows, requested) is None + assert _serve_from_cache(stored, rows, stored) == rows + + +def test_rollup_hit_sorts_null_dates_last_before_the_limit() -> None: + """A rolled-up page keeps the earliest dates and leaves NULL dates for last.""" + stored_query: SemanticQuery = SemanticQuery( + metrics=[SUMMED_SALES], dimensions=[REGION, ORDER_DATE] + ) + stored_rows: dict[str, list[object]] = { + "region": ["East", "West", "East", "West"], + "order_date": [None, date(2026, 2, 1), date(2026, 1, 1), None], + "total_sales": [7.0, 2.0, 1.0, 9.0], + } + query: SemanticQuery = SemanticQuery( + metrics=[SUMMED_SALES], dimensions=[ORDER_DATE], limit=2 + ) + + assert _serve_from_cache(stored_query, stored_rows, query) == { + "order_date": [date(2026, 1, 1), date(2026, 2, 1)], + "total_sales": [1.0, 2.0], + } + assert _serve_from_cache(stored_query, stored_rows, replace(query, limit=None)) == { + "order_date": [date(2026, 1, 1), date(2026, 2, 1), None], + "total_sales": [1.0, 2.0, 16.0], + } diff --git a/tests/unit_tests/semantic_layers/mapper_test.py b/tests/unit_tests/semantic_layers/mapper_test.py index 32bd0c750a8f..e5c76cc9e625 100644 --- a/tests/unit_tests/semantic_layers/mapper_test.py +++ b/tests/unit_tests/semantic_layers/mapper_test.py @@ -39,6 +39,8 @@ Grains, GroupLimit, Metric, + null_placement, + NullPlacement, Operator, OrderDirection, PredicateType, @@ -3531,14 +3533,20 @@ def test_get_results_with_is_rowcount( @pytest.mark.parametrize( "is_rowcount, expected_order", - [(False, [("orders.order_date", OrderDirection.ASC)]), (True, None)], + [ + ( + False, + [("orders.order_date", OrderDirection.ASC, NullPlacement.LAST)], + ), + (True, None), + ], ids=["table", "row-count"], ) def test_default_order_applies_to_table_dispatches_only( mock_datasource: MagicMock, mocker: MockerFixture, is_rowcount: bool, - expected_order: list[tuple[str, OrderDirection]] | None, + expected_order: list[tuple[str, OrderDirection, NullPlacement | None]] | None, ) -> None: """Order cannot change a row count, so a count query is sent unordered.""" order_date: Dimension = Dimension( @@ -3565,7 +3573,7 @@ def test_default_order_applies_to_table_dispatches_only( received: SemanticQuery = provider.call_args.args[0] assert ( - [(element.id, direction) for element, direction in received.order] + [(term[0].id, term[1], null_placement(term)) for term in received.order] if received.order else None ) == expected_order From 2959e13c86b33ee04a18e4b0d64b28e64fa5a6e7 Mon Sep 17 00:00:00 2001 From: Mike Bridge Date: Fri, 30 Oct 2026 11:07:59 -0600 Subject: [PATCH 2/3] fix(semantic-layer): keep order terms usable with dataclasses.asdict dataclasses.asdict and astuple rebuild a tuple from its elements, which failed for OrderTerm. The term now takes their named-tuple path, keeping its member and direction; its docstring says the placement is not carried and to read it with null_placement. The conformance kit names a missing query case instead of failing with a bare KeyError, and the provider testing guide asks the null_placement fixture for distinct key values and a single NULL group, since tied rows may come back in either order. --- .../semantic-layer-provider-testing.md | 4 +++- .../superset_core/semantic_layers/testing.py | 4 ++++ .../src/superset_core/semantic_layers/types.py | 6 ++++++ .../tests/semantic_layers/test_types.py | 18 +++++++++++++++--- 4 files changed, 28 insertions(+), 4 deletions(-) diff --git a/docs/developer_docs/semantic-layer-provider-testing.md b/docs/developer_docs/semantic-layer-provider-testing.md index 3e68525c04ef..3053e0d78046 100644 --- a/docs/developer_docs/semantic-layer-provider-testing.md +++ b/docs/developer_docs/semantic-layer-provider-testing.md @@ -68,7 +68,9 @@ seed NULL and non-NULL values of that member so the limited page keeps different rows under the opposite placement. The SQLite example sorts `category` ascending with NULL values last and a limit of 2, keeping A and B where SQLite's own default would keep NULL and A. The suite also checks that the unlimited result -puts every NULL value of that member at the requested end. +puts every NULL value of that member at the requested end, and compares the +limited page with the same rows of the unlimited result, so give the order key +distinct values and a single NULL group: tied rows may come back in either order. For `grain`, use a grain-bearing dimension with data whose aggregation distinguishes the selected grain from raw values. Multiple grain variants for one name are allowed, not required. When the catalog declares no grain-bearing dimension, the case skips with an explicit reason before looking up its fixture entry; such providers may omit the `grain` entry. This reports unexercised grain coverage, not a passing grain check. NULL and grouped-OR fixtures must select NULL and non-NULL branches observably; the `or` case must use the host-normalized shape: one `OrFilter` containing diff --git a/superset-core/src/superset_core/semantic_layers/testing.py b/superset-core/src/superset_core/semantic_layers/testing.py index 31d7b1f048b6..713f62bde44f 100644 --- a/superset-core/src/superset_core/semantic_layers/testing.py +++ b/superset-core/src/superset_core/semantic_layers/testing.py @@ -128,6 +128,10 @@ def test_query_outputs( pytest.skip("grain: catalog declares no grain-bearing dimension") query: SemanticQuery expected: pa.Table + assert case in semantic_query_cases, ( + f"semantic_query_cases has no {case!r} case; see the provider " + "testing guide for the required cases" + ) query, expected = semantic_query_cases[case] assert expected.num_rows > 0, ( f"{case} must distinguish a correct result from no data" diff --git a/superset-core/src/superset_core/semantic_layers/types.py b/superset-core/src/superset_core/semantic_layers/types.py index 1e831d09c0ca..2c2d7e85da19 100644 --- a/superset-core/src/superset_core/semantic_layers/types.py +++ b/superset-core/src/superset_core/semantic_layers/types.py @@ -337,8 +337,14 @@ class OrderTerm(tuple[Metric | Dimension | AdhocExpression, OrderDirection]): plain pair, and such a term equals the plain pair. A term with a placement differs from the pair without one: the provider must sort NULL values of that key first or last, before ``offset`` and ``limit``. + + ``dataclasses.asdict`` and ``astuple`` rebuild a term from its two + elements, so they keep the member and direction but not the placement; + serialize a term with :func:`null_placement` to keep it. """ + # Lets ``dataclasses.asdict``/``astuple`` rebuild a term from its elements. + _fields: tuple[str, str] = ("member", "direction") nulls: NullPlacement | None def __new__( diff --git a/superset-core/tests/semantic_layers/test_types.py b/superset-core/tests/semantic_layers/test_types.py index ddc52c0d863d..76ccea1e98e7 100644 --- a/superset-core/tests/semantic_layers/test_types.py +++ b/superset-core/tests/semantic_layers/test_types.py @@ -17,7 +17,7 @@ import copy import pickle -from dataclasses import replace +from dataclasses import asdict, astuple, replace import pyarrow as pa import pytest @@ -156,19 +156,31 @@ def test_order_term_placement_distinguishes_it_from_the_plain_pair( def test_order_term_survives_copy_and_pickle() -> None: term = OrderTerm(ORDER_DATE, OrderDirection.DESC, NullPlacement.FIRST) - query = replace(SemanticQuery([], [ORDER_DATE], order=[term])) + query = SemanticQuery([], [ORDER_DATE], order=[term]) for restored in ( copy.copy(term), copy.deepcopy(term), pickle.loads(pickle.dumps(term)), # noqa: S301 - *(query.order or []), + *(copy.deepcopy(query).order or []), + *(pickle.loads(pickle.dumps(query)).order or []), # noqa: S301 + *(replace(query, limit=1).order or []), ): assert type(restored) is OrderTerm assert restored == term assert null_placement(restored) is NullPlacement.FIRST +def test_asdict_keeps_member_and_direction_of_an_order_term() -> None: + """``asdict`` cannot carry the placement; it must not fail on a term.""" + term = OrderTerm(ORDER_DATE, OrderDirection.ASC, NullPlacement.LAST) + + order = asdict(SemanticQuery([], [ORDER_DATE], order=[term]))["order"] + + assert [tuple(item) for item in order] == [(asdict(ORDER_DATE), OrderDirection.ASC)] + assert astuple(SemanticQuery([], [ORDER_DATE], order=[term])) + + def test_order_term_is_immutable_and_validated() -> None: term = OrderTerm(ORDER_DATE, OrderDirection.ASC, NullPlacement.LAST) From 762801b8a88065823edcd04c06a145cd1a11b9cd Mon Sep 17 00:00:00 2001 From: Mike Bridge Date: Mon, 2 Nov 2026 16:47:23 -0600 Subject: [PATCH 3/3] test(semantic-layer): witness secondary null placement --- UPDATING.md | 3 +- .../semantic-layer-provider-testing.md | 9 +- .../superset_core/semantic_layers/testing.py | 78 +++++++++---- .../tests/semantic_layers/test_conformance.py | 109 ++++++++++++++++++ .../tests/semantic_layers/test_types.py | 29 +++-- superset/semantic_layers/cache_identity.py | 4 + .../semantic_layers/default_order_test.py | 6 +- 7 files changed, 200 insertions(+), 38 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index f511b42a3ea7..dd4f0fbb7a93 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -84,7 +84,8 @@ assists people when migrating to a new version. provider must apply the placement (read it with `null_placement(term)`) before `offset` and `limit`, and keep it when it rebuilds order terms. Explicit orders are sent unchanged and keep the provider's own NULL placement. The provider - conformance kit adds a required `null_placement` query case. The semantic + conformance kit adds required `null_placement` and `null_placement_secondary` + query cases, covering the first key and a tied-prefix secondary key. The semantic result cache key format changes, so cached semantic results are recomputed once after upgrading. diff --git a/docs/developer_docs/semantic-layer-provider-testing.md b/docs/developer_docs/semantic-layer-provider-testing.md index 3053e0d78046..4bb68da46d03 100644 --- a/docs/developer_docs/semantic-layer-provider-testing.md +++ b/docs/developer_docs/semantic-layer-provider-testing.md @@ -60,7 +60,7 @@ separate integration tests. | `semantic_provider_class` | `(importable_module_name, class_name)` of the layer for an isolated import and static-schema check. Dependencies must already be installed. | Required query cases are `identity`, `grain`, `null`, `not_null`, `or`, -`order_limit` and `null_placement`. Use member names distinct from private IDs +`order_limit`, `null_placement` and `null_placement_secondary`. Use member names distinct from private IDs and display labels. For `null_placement`, order first by a selected dimension with an `OrderTerm(member, direction, NullPlacement.LAST)` (or `FIRST`) and a limit, and @@ -71,6 +71,13 @@ default would keep NULL and A. The suite also checks that the unlimited result puts every NULL value of that member at the requested end, and compares the limited page with the same rows of the unlimited result, so give the order key distinct values and a single NULL group: tied rows may come back in either order. +For `null_placement_secondary`, use at least two order keys, with a selected +dimension carrying explicit NULL placement on the second key. Seed NULL and +non-NULL values within a tie on the first key, and put the page boundary inside +that tie so reversing only the second key's placement changes the retained rows. +The example orders by month then category, skips January's first row and keeps +its NULL category (total 3); dropping secondary `NULLS LAST` instead keeps A +(total 2). The suite checks placement within each tied prefix, not globally. For `grain`, use a grain-bearing dimension with data whose aggregation distinguishes the selected grain from raw values. Multiple grain variants for one name are allowed, not required. When the catalog declares no grain-bearing dimension, the case skips with an explicit reason before looking up its fixture entry; such providers may omit the `grain` entry. This reports unexercised grain coverage, not a passing grain check. NULL and grouped-OR fixtures must select NULL and non-NULL branches observably; the `or` case must use the host-normalized shape: one `OrFilter` containing diff --git a/superset-core/src/superset_core/semantic_layers/testing.py b/superset-core/src/superset_core/semantic_layers/testing.py index 713f62bde44f..372706a06060 100644 --- a/superset-core/src/superset_core/semantic_layers/testing.py +++ b/superset-core/src/superset_core/semantic_layers/testing.py @@ -113,6 +113,7 @@ def test_catalog_identity( "or", "order_limit", "null_placement", + "null_placement_secondary", ], ) def test_query_outputs( @@ -164,8 +165,13 @@ def test_query_outputs( _assert_result_equal( full.results.slice(query.offset or 0, query.limit), expected ) - elif case == "null_placement": - _assert_null_placement_witness(query, semantic_view, expected) + elif case in {"null_placement", "null_placement_secondary"}: + _assert_null_placement_witness( + query, + semantic_view, + expected, + order_index=1 if case == "null_placement_secondary" else 0, + ) result: SemanticResult = semantic_view.get_table(query) _assert_result_equal(result.results, expected) assert set(result.results.column_names) == { @@ -291,51 +297,77 @@ def test_imports_and_static_schema( def _assert_null_placement_witness( - query: SemanticQuery, view: SemanticView, expected: pa.Table + query: SemanticQuery, + view: SemanticView, + expected: pa.Table, + *, + order_index: int = 0, ) -> None: - """Require NULL values of the first order key where its placement puts them. + """Require a page that observes the selected order key's NULL placement. - The host's default chronological order asks for NULL values last; a - provider must honor a placement on any order key, before offset and limit. - The page must keep different rows from the ones the opposite placement would. + Placement on a later key applies within ties of all preceding keys. + Reversing it within those ties must change which rows the page keeps. """ assert query.order, "null_placement fixture requires an order" - first: tuple[object, ...] = query.order[0] - placement: NullPlacement | None = null_placement(first) + assert len(query.order) > order_index, ( + "null_placement fixture requires the selected order key" + ) + term: tuple[object, ...] = query.order[order_index] + placement: NullPlacement | None = null_placement(term) assert placement is not None, ( - "null_placement fixture's first order key requires a NullPlacement" + "null_placement fixture's selected order key requires a NullPlacement" ) - member: object = first[0] + member: object = term[0] assert isinstance(member, Dimension), ( "null_placement fixture must order by a selected dimension" ) assert member in query.dimensions, ( "null_placement fixture must order by a selected dimension" ) + prefix: list[str] = [] + key: object + for key, _ in query.order[:order_index]: + assert isinstance(key, (Dimension, Metric)) + assert key in [*query.dimensions, *query.metrics] + prefix.append(key.name) assert query.limit is not None assert query.limit > 0 full: pa.Table = view.get_table(replace(query, limit=None, offset=None)).results + rows: list[dict[str, object]] = full.to_pylist() values: list[object] = full.column(member.name).to_pylist() - nulls: list[object] = [value for value in values if value is None] - others: list[object] = [value for value in values if value is not None] - assert nulls, "null_placement fixture requires NULL values of the order key" - assert others, "null_placement fixture requires non-NULL values of the order key" - placed: list[object] = ( - others + nulls if placement is NullPlacement.LAST else nulls + others + assert any(value is None for value in values), ( + "null_placement fixture requires NULL values of the order key" ) - opposite: list[object] = ( - nulls + others if placement is NullPlacement.LAST else others + nulls + assert any(value is not None for value in values), ( + "null_placement fixture requires non-NULL values of the order key" ) - assert values == placed, ( + groups: dict[tuple[object, ...], list[int]] = {} + index: int + row: dict[str, object] + for index, row in enumerate(rows): + groups.setdefault(tuple(row[name] for name in prefix), []).append(index) + placed: list[int] = [] + opposite: list[int] = [] + indices: list[int] + for indices in groups.values(): + nulls: list[int] = [index for index in indices if values[index] is None] + others: list[int] = [index for index in indices if values[index] is not None] + placed.extend( + others + nulls if placement is NullPlacement.LAST else nulls + others + ) + opposite.extend( + nulls + others if placement is NullPlacement.LAST else others + nulls + ) + assert placed == list(range(full.num_rows)), ( f"{member.name}: NULL values must sort {placement.value.lower()}, got {values}" ) start: int = query.offset or 0 stop: int = start + query.limit - assert sorted(map(repr, placed[start:stop])) != sorted( - map(repr, opposite[start:stop]) + assert sorted(repr(rows[index]) for index in placed[start:stop]) != sorted( + repr(rows[index]) for index in opposite[start:stop] ), ( "null_placement fixture's page must keep different rows " - "under the opposite placement" + "under the opposite placement within tied order prefixes" ) _assert_result_equal(full.slice(start, query.limit), expected) diff --git a/superset-core/tests/semantic_layers/test_conformance.py b/superset-core/tests/semantic_layers/test_conformance.py index 41374fd1f65e..0be9a0d82084 100644 --- a/superset-core/tests/semantic_layers/test_conformance.py +++ b/superset-core/tests/semantic_layers/test_conformance.py @@ -36,6 +36,7 @@ Operator, OrderDirection, OrderTerm, + OrderTuple, OrFilter, PredicateType, SemanticQuery, @@ -153,6 +154,26 @@ def semantic_query_cases( ), pa.table({"category": ["B"], "total": [8]}), ), + # January ties on the first key: its second row is NULL, not A. + "null_placement_secondary": ( + SemanticQuery( + [total], + [month, category], + order=[ + OrderTerm(month, OrderDirection.ASC, NullPlacement.LAST), + OrderTerm(category, OrderDirection.ASC, NullPlacement.LAST), + ], + offset=1, + limit=1, + ), + pa.table( + { + "occurred": pa.array([date(2026, 1, 1)], type=pa.date32()), + "category": pa.array([None], type=pa.string()), + "total": [3], + } + ), + ), # SQLite sorts NULL first for ASC: ignoring the placement keeps NULL, A. "null_placement": ( SemanticQuery( @@ -720,3 +741,91 @@ def drop_placement(query: SemanticQuery) -> SemanticResult: SemanticProviderConformance().test_query_outputs( "null_placement", semantic_view, semantic_query_cases ) + + +@pytest.mark.parametrize( + ("direction", "placement"), + [ + (OrderDirection.ASC, NullPlacement.LAST), + (OrderDirection.DESC, NullPlacement.FIRST), + ], +) +def test_secondary_null_placement_rejects_first_key_only_provider( + semantic_view: "QueryView", + semantic_query_cases: dict[str, tuple[SemanticQuery, pa.Table]], + monkeypatch: pytest.MonkeyPatch, + direction: OrderDirection, + placement: NullPlacement, +) -> None: + """Secondary placement must select the right page within a tied prefix.""" + query: SemanticQuery + expected: pa.Table + query, expected = semantic_query_cases["null_placement_secondary"] + assert query.order + category: Dimension = query.dimensions[1] + query = replace( + query, order=[query.order[0], OrderTerm(category, direction, placement)] + ) + if placement is NullPlacement.FIRST: + expected = pa.table( + { + "occurred": pa.array([date(2026, 1, 1)], type=pa.date32()), + "category": ["A"], + "total": [2], + } + ) + cases: dict[str, tuple[SemanticQuery, pa.Table]] = { + "null_placement_secondary": (query, expected), + } + SemanticProviderConformance().test_query_outputs( + "null_placement_secondary", semantic_view, cases + ) + original: Callable[[SemanticQuery], SemanticResult] = semantic_view.get_table + + def first_key_only(value: SemanticQuery) -> SemanticResult: + return original( + replace( + value, + order=[ + term if index == 0 else (term[0], term[1]) + for index, term in enumerate(value.order or []) + ], + ) + ) + + monkeypatch.setattr(semantic_view, "get_table", first_key_only) + with pytest.raises(AssertionError, match="NULL values must sort"): + SemanticProviderConformance().test_query_outputs( + "null_placement_secondary", semantic_view, cases + ) + + +@pytest.mark.parametrize( + "mutation", ["first_key_only", "plain_pair", "whole_result", "no_tied_nulls"] +) +def test_secondary_null_placement_rejects_weak_fixtures( + semantic_view: "QueryView", + semantic_query_cases: dict[str, tuple[SemanticQuery, pa.Table]], + mutation: str, +) -> None: + """A secondary witness must expose placement within a tied prefix.""" + query: SemanticQuery + expected: pa.Table + query, expected = semantic_query_cases["null_placement_secondary"] + assert query.order + order: list[OrderTuple] = query.order + if mutation == "first_key_only": + query = replace(query, order=order[:1]) + elif mutation == "plain_pair": + query = replace(query, order=[order[0], (order[1][0], order[1][1])]) + elif mutation == "whole_result": + query = replace(query, limit=100, offset=None) + else: + # A category cannot be NULL and non-NULL within a tie on itself. + query = replace(query, order=[order[1], order[1]]) + with pytest.raises(AssertionError, match="requires|must keep different rows"): + SemanticProviderConformance().test_query_outputs( + "null_placement_secondary", + semantic_view, + {"null_placement_secondary": (query, expected)}, + ) diff --git a/superset-core/tests/semantic_layers/test_types.py b/superset-core/tests/semantic_layers/test_types.py index 76ccea1e98e7..4f405b6625b5 100644 --- a/superset-core/tests/semantic_layers/test_types.py +++ b/superset-core/tests/semantic_layers/test_types.py @@ -22,12 +22,14 @@ import pyarrow as pa import pytest from superset_core.semantic_layers.types import ( + AdhocExpression, Dimension, Metric, null_placement, NullPlacement, OrderDirection, OrderTerm, + OrderTuple, SemanticQuery, ) @@ -110,12 +112,14 @@ def test_metadata_defaults_are_not_shared() -> None: assert second.metadata == {} -ORDER_DATE = Dimension("orders.order_date", "order_date", pa.date32()) +ORDER_DATE: Dimension = Dimension("orders.order_date", "order_date", pa.date32()) def test_order_term_reads_like_a_plain_pair() -> None: - term = OrderTerm(ORDER_DATE, OrderDirection.ASC, NullPlacement.LAST) + term: OrderTerm = OrderTerm(ORDER_DATE, OrderDirection.ASC, NullPlacement.LAST) + member: Metric | Dimension | AdhocExpression + direction: OrderDirection member, direction = term assert (member, direction) == (ORDER_DATE, OrderDirection.ASC) @@ -127,8 +131,8 @@ def test_order_term_reads_like_a_plain_pair() -> None: def test_order_term_without_placement_equals_the_plain_pair() -> None: - pair = (ORDER_DATE, OrderDirection.ASC) - term = OrderTerm(ORDER_DATE, OrderDirection.ASC) + pair: OrderTuple = (ORDER_DATE, OrderDirection.ASC) + term: OrderTerm = OrderTerm(ORDER_DATE, OrderDirection.ASC) assert term == pair assert pair == term @@ -140,8 +144,8 @@ def test_order_term_without_placement_equals_the_plain_pair() -> None: def test_order_term_placement_distinguishes_it_from_the_plain_pair( nulls: NullPlacement, ) -> None: - pair = (ORDER_DATE, OrderDirection.ASC) - term = OrderTerm(ORDER_DATE, OrderDirection.ASC, nulls) + pair: OrderTuple = (ORDER_DATE, OrderDirection.ASC) + term: OrderTerm = OrderTerm(ORDER_DATE, OrderDirection.ASC, nulls) assert term != pair assert pair != term @@ -155,9 +159,10 @@ def test_order_term_placement_distinguishes_it_from_the_plain_pair( def test_order_term_survives_copy_and_pickle() -> None: - term = OrderTerm(ORDER_DATE, OrderDirection.DESC, NullPlacement.FIRST) - query = SemanticQuery([], [ORDER_DATE], order=[term]) + term: OrderTerm = OrderTerm(ORDER_DATE, OrderDirection.DESC, NullPlacement.FIRST) + query: SemanticQuery = SemanticQuery([], [ORDER_DATE], order=[term]) + restored: OrderTuple for restored in ( copy.copy(term), copy.deepcopy(term), @@ -173,16 +178,18 @@ def test_order_term_survives_copy_and_pickle() -> None: def test_asdict_keeps_member_and_direction_of_an_order_term() -> None: """``asdict`` cannot carry the placement; it must not fail on a term.""" - term = OrderTerm(ORDER_DATE, OrderDirection.ASC, NullPlacement.LAST) + term: OrderTerm = OrderTerm(ORDER_DATE, OrderDirection.ASC, NullPlacement.LAST) - order = asdict(SemanticQuery([], [ORDER_DATE], order=[term]))["order"] + order: list[tuple[object, OrderDirection]] = asdict( + SemanticQuery([], [ORDER_DATE], order=[term]) + )["order"] assert [tuple(item) for item in order] == [(asdict(ORDER_DATE), OrderDirection.ASC)] assert astuple(SemanticQuery([], [ORDER_DATE], order=[term])) def test_order_term_is_immutable_and_validated() -> None: - term = OrderTerm(ORDER_DATE, OrderDirection.ASC, NullPlacement.LAST) + term: OrderTerm = OrderTerm(ORDER_DATE, OrderDirection.ASC, NullPlacement.LAST) with pytest.raises(AttributeError): term.nulls = None diff --git a/superset/semantic_layers/cache_identity.py b/superset/semantic_layers/cache_identity.py index 0bbdc9238c23..6bbc8555c5ef 100644 --- a/superset/semantic_layers/cache_identity.py +++ b/superset/semantic_layers/cache_identity.py @@ -34,6 +34,7 @@ Metric, null_placement, NullPlacement, + OrderDirection, OrderTuple, OrFilter, SemanticQuery, @@ -213,6 +214,9 @@ def _canonical_query(query: SemanticQuery) -> Mapping[str, object]: def _canonical_order(order: list[OrderTuple]) -> list[Mapping[str, object]]: """Serialize order terms, including NULL placement when a term sets one.""" canonical: list[Mapping[str, object]] = [] + term: OrderTuple + element: Metric | Dimension | AdhocExpression + direction: OrderDirection for term in order: element, direction = term nulls: NullPlacement | None = null_placement(term) diff --git a/tests/unit_tests/semantic_layers/default_order_test.py b/tests/unit_tests/semantic_layers/default_order_test.py index 1f21829c5486..28b36597c1ca 100644 --- a/tests/unit_tests/semantic_layers/default_order_test.py +++ b/tests/unit_tests/semantic_layers/default_order_test.py @@ -194,8 +194,8 @@ def test_row_count_dispatch_bypasses_the_cache(mocker: MockerFixture) -> None: "build_cache_configuration", return_value=(mocker.Mock(), mocker.Mock()), ) - service.execute_provider.side_effect = lambda query, dispatch: ( - SemanticCacheOutcome(dispatch(query), cache_hit=False) + service.execute_provider.side_effect = lambda query, dispatch: SemanticCacheOutcome( + dispatch(query), cache_hit=False ) provider: Mock = mocker.Mock(return_value=EMPTY_RESULT) query: SemanticQuery = SemanticQuery(metrics=[SALES], dimensions=[ORDER_DATE]) @@ -433,6 +433,8 @@ def test_limited_hit_is_not_shared_between_default_and_explicit_ascending( "order_date": [date(2026, 1, 1), date(2026, 2, 1)], "total_sales": [1.0, 2.0], } + stored: SemanticQuery + requested: SemanticQuery stored, requested = ( (explicit, unordered) if stored_explicit