diff --git a/UPDATING.md b/UPDATING.md index 3e564449ed8d..dd4f0fbb7a93 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -75,6 +75,20 @@ 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 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. + - `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..4bb68da46d03 100644 --- a/docs/developer_docs/semantic-layer-provider-testing.md +++ b/docs/developer_docs/semantic-layer-provider-testing.md @@ -59,8 +59,25 @@ 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`, `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 +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, 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 @@ -108,9 +125,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..372706a06060 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,17 @@ 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", + "null_placement_secondary", + ], ) def test_query_outputs( self, @@ -116,6 +129,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" @@ -148,6 +165,13 @@ def test_query_outputs( _assert_result_equal( full.results.slice(query.offset or 0, query.limit), 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) == { @@ -272,6 +296,82 @@ 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, + *, + order_index: int = 0, +) -> None: + """Require a page that observes the selected order key's NULL placement. + + 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" + 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 selected order key requires a NullPlacement" + ) + 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() + assert any(value is None for value in values), ( + "null_placement fixture requires NULL values of the order key" + ) + assert any(value is not None for value in values), ( + "null_placement fixture requires non-NULL values of the order key" + ) + 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(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 within tied order prefixes" + ) + _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..2c2d7e85da19 100644 --- a/superset-core/src/superset_core/semantic_layers/types.py +++ b/superset-core/src/superset_core/semantic_layers/types.py @@ -315,9 +315,91 @@ 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``. + + ``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__( + 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 +456,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..0be9a0d82084 100644 --- a/superset-core/tests/semantic_layers/test_conformance.py +++ b/superset-core/tests/semantic_layers/test_conformance.py @@ -32,8 +32,11 @@ FilterExpression, Grains, Metric, + NullPlacement, Operator, OrderDirection, + OrderTerm, + OrderTuple, OrFilter, PredicateType, SemanticQuery, @@ -151,6 +154,36 @@ 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( + [total], + [category], + order=[OrderTerm(category, OrderDirection.ASC, NullPlacement.LAST)], + limit=2, + ), + pa.table({"category": ["A", "B"], "total": [7, 8]}), + ), } @@ -646,3 +679,153 @@ 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 + ) + + +@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 15e74c862bf3..4f405b6625b5 100644 --- a/superset-core/tests/semantic_layers/test_types.py +++ b/superset-core/tests/semantic_layers/test_types.py @@ -15,8 +15,23 @@ # specific language governing permissions and limitations # under the License. +import copy +import pickle +from dataclasses import asdict, astuple, replace + import pyarrow as pa -from superset_core.semantic_layers.types import Dimension, Metric +import pytest +from superset_core.semantic_layers.types import ( + AdhocExpression, + Dimension, + Metric, + null_placement, + NullPlacement, + OrderDirection, + OrderTerm, + OrderTuple, + SemanticQuery, +) def test_dimension_metadata_is_not_part_of_identity() -> None: @@ -95,3 +110,94 @@ def test_metadata_defaults_are_not_shared() -> None: first.metadata["display_name"] = "First" assert second.metadata == {} + + +ORDER_DATE: Dimension = Dimension("orders.order_date", "order_date", pa.date32()) + + +def test_order_term_reads_like_a_plain_pair() -> None: + 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) + 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: OrderTuple = (ORDER_DATE, OrderDirection.ASC) + term: OrderTerm = 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: OrderTuple = (ORDER_DATE, OrderDirection.ASC) + term: OrderTerm = 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 = 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), + pickle.loads(pickle.dumps(term)), # noqa: S301 + *(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 = OrderTerm(ORDER_DATE, OrderDirection.ASC, NullPlacement.LAST) + + 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 = 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..6bbc8555c5ef 100644 --- a/superset/semantic_layers/cache_identity.py +++ b/superset/semantic_layers/cache_identity.py @@ -32,6 +32,9 @@ FilterExpression, GroupLimit, Metric, + null_placement, + NullPlacement, + OrderDirection, OrderTuple, OrFilter, SemanticQuery, @@ -41,7 +44,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 +164,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 +204,32 @@ 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]] = [] + term: OrderTuple + element: Metric | Dimension | AdhocExpression + direction: OrderDirection + 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..28b36597c1ca 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: @@ -156,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]) @@ -374,3 +412,61 @@ 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: SemanticQuery + requested: SemanticQuery + 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