mikebridge commented on code in PR #45912:
URL: https://github.com/apache/superset/pull/45912#discussion_r4457957618
##########
superset-core/src/superset_core/semantic_layers/testing.py:
##########
@@ -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)
Review Comment:
Fixed in f2dba096fa. Prefix groups now retain Arrow scalars rather than
converting nested values to Python dicts/lists. The STRUCT and LIST regressions
put equal values in separate chunks, assert scalar equality and equal hashes,
then exercise the tied-prefix witness. I also checked this directly on PyArrow
25.0.1. Both cases raised TypeError before the fix and pass now.
The complete PR test set passes: 1,989 tests, with 4 capability skips; hooks
pass across the full PR file set.
##########
superset-core/src/superset_core/semantic_layers/testing.py:
##########
@@ -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]
Review Comment:
Fixed in f2dba096fa. The page check compares multisets of prefix/order-key
values instead of full rows, so payload differences inside an unordered NULL
tie cannot make a weak fixture appear discriminating.
Added first-key and secondary-key versions of (A,x), (NULL,y), (NULL,z) at
offset 1 / limit 1: both were wrongly accepted before the change and are
rejected now. The existing first-key-only placement mutation still fails the
required secondary case while all 65 original tests pass. The full PR test set
passes: 1,989 tests, with 4 capability skips.
--
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]
