aminghadersohi commented on code in PR #45912:
URL: https://github.com/apache/superset/pull/45912#discussion_r4463059402
##########
superset-core/src/superset_core/semantic_layers/testing.py:
##########
@@ -370,6 +396,128 @@ 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: OrderTuple = 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"
+ )
+ order_names: list[str] = []
+ key: object
+ for key, _ in query.order:
+ assert isinstance(key, (Dimension, Metric))
+ assert key in [*query.dimensions, *query.metrics]
+ order_names.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[pa.Scalar | None] = [
+ value if value.is_valid else None for value in full.column(member.name)
+ ]
+ 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(
+ value.value if isinstance(value, pa.DictionaryScalar) else value
+ for value in (full.column(name)[index] for name in order_names)
+ )
+ 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][:order_index], []).append(index)
+ if order_index:
+ assert len(groups) > 1, "secondary placement requires multiple prefix
groups"
+ assert any(
+ values[index] is None
+ for indices in list(groups.values())[:-1]
+ for index in indices
+ ), "secondary placement requires NULLs before the final prefix group"
Review Comment:
This rule assumes `NULLS LAST`. With `NULLS FIRST`, NULLs only in the first
prefix group pass and let a provider that moves NULLs to the start of the whole
result through, while NULLs only in the final group, which would expose it, are
rejected.
```suggestion
if order_index:
assert len(groups) > 1, "secondary placement requires multiple
prefix groups"
# Global placement moves NULLs to the requested end of the whole
result.
last: bool = placement is NullPlacement.LAST
ordered: list[list[int]] = list(groups.values())
movable: list[list[int]] = ordered[:-1] if last else ordered[1:]
side: str = "before the final" if last else "after the first"
assert any(values[index] is None for group in movable for index in
group), (
f"secondary placement requires NULLs {side} prefix group"
)
```
##########
superset-core/src/superset_core/semantic_layers/testing.py:
##########
@@ -370,6 +396,128 @@ 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: OrderTuple = 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"
+ )
+ order_names: list[str] = []
+ key: object
+ for key, _ in query.order:
+ assert isinstance(key, (Dimension, Metric))
+ assert key in [*query.dimensions, *query.metrics]
+ order_names.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[pa.Scalar | None] = [
+ value if value.is_valid else None for value in full.column(member.name)
+ ]
Review Comment:
Nit: A dictionary column built with `null_encoding='encode'` stores NULL as
a valid index, so `is_valid` is True and a correctly ordered provider is
rejected with 'requires NULL values of the order key'. `is_null()` sees those
entries.
```suggestion
# ``is_null`` also sees NULL entries stored in a dictionary.
null_mask: list[bool] = full.column(member.name).is_null().to_pylist()
values: list[pa.Scalar | None] = [
None if is_null else value
for value, is_null in zip(full.column(member.name), null_mask,
strict=True)
]
```
--
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]
