mikebridge commented on code in PR #45912:
URL: https://github.com/apache/superset/pull/45912#discussion_r4459587383
##########
superset-core/src/superset_core/semantic_layers/testing.py:
##########
@@ -272,6 +297,86 @@ 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
+ 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"
+ )
+ # Arrow scalars keep nested prefix values hashable, including across
chunks.
+ order_values: list[tuple[pa.Scalar, ...]] = [
+ tuple(full.column(name)[index] for name in [*prefix, member.name])
+ for index in range(full.num_rows)
+ ]
+ groups: dict[tuple[pa.Scalar, ...], list[int]] = {}
+ index: int
+ for index in range(full.num_rows):
+ groups.setdefault(order_values[index][:-1], []).append(index)
Review Comment:
Fixed in 820e4c4444. The witness retains every declared selected order key,
including trailing tie-breakers, and groups only by the keys preceding the
placement being checked. Added first-key and secondary-key versions of your
category/payload example: both were rejected before the fix and pass now. The
prior case without a declared payload key still gets rejected as
non-discriminating.
Conformance self-tests: 80 passed, 4 capability skips. The full PR test set
passes: 1,993 tests, with 4 skips; hooks pass across the full PR file set.
##########
superset-core/src/superset_core/semantic_layers/testing.py:
##########
@@ -272,6 +297,86 @@ 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
+ 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"
+ )
+ # Arrow scalars keep nested prefix values hashable, including across
chunks.
+ order_values: list[tuple[pa.Scalar, ...]] = [
+ tuple(full.column(name)[index] for name in [*prefix, member.name])
+ for index in range(full.num_rows)
+ ]
+ groups: dict[tuple[pa.Scalar, ...], list[int]] = {}
+ index: int
+ for index in range(full.num_rows):
+ groups.setdefault(order_values[index][:-1], []).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
+ # Payload differences within an unordered key tie cannot distinguish pages.
+ assert Counter(order_values[index] for index in placed[start:stop]) !=
Counter(
+ order_values[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)
Review Comment:
Fixed in 820e4c4444. Each required lane now executes the same limited query
with only the tested key’s NULL placement reversed, then checks its complete
order-key values against the opposite page derived from the unlimited fixture.
Metrics, dimensions, filters, directions, offset and limit stay the same.
Added NULLS FIRST fixtures for both required cases, with the real SQLite
provider as a passing control. A wrapper that drops every placement passed
before the fix and is now rejected in both cases. The full PR test set passes:
1,993 tests, with 4 capability skips; conformance self-tests and hooks also
pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
