aminghadersohi commented on code in PR #45912:
URL: https://github.com/apache/superset/pull/45912#discussion_r4465596280
##########
superset-core/src/superset_core/semantic_layers/testing.py:
##########
@@ -370,6 +396,135 @@ 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
+ column: pa.ChunkedArray = full.column(member.name)
+ if pa.types.is_dictionary(column.type):
+ column = column.cast(column.type.value_type)
+ null_mask: list[bool] = column.is_null().to_pylist()
+ values: list[pa.Scalar | None] = [
+ None if is_null else value
+ for value, is_null in zip(column, null_mask, strict=True)
+ ]
+ 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"
+ 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"
+ )
Review Comment:
A NULL before the final group is not enough: if every later group is
all-NULL (or every earlier group for FIRST), global and per-prefix placement
match, so a provider that emulates only one placement globally passes. The rule
needs a NULL group before a later non-NULL group.
```suggestion
ordered: list[list[int]] = list(groups.values())
has_null: list[bool] = [any(values[i] is None for i in g) for g in
ordered]
has_value: list[bool] = [any(values[i] is not None for i in g) for g
in ordered]
side: str = "before the final" if last else "after the first"
# Global placement must move a NULL past a non-NULL value of another
prefix.
assert any(
has_null[a] and has_value[b] if last else has_value[a] and
has_null[b]
for b in range(len(ordered))
for a in range(b)
), f"secondary placement requires NULLs {side} prefix group, past a
non-NULL"
```
##########
superset-core/src/superset_core/semantic_layers/testing.py:
##########
@@ -370,6 +396,135 @@ 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
+ column: pa.ChunkedArray = full.column(member.name)
+ if pa.types.is_dictionary(column.type):
+ column = column.cast(column.type.value_type)
+ null_mask: list[bool] = column.is_null().to_pylist()
+ values: list[pa.Scalar | None] = [
+ None if is_null else value
+ for value, is_null in zip(column, null_mask, strict=True)
+ ]
+ 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"
+ 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"
+ )
+ 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)
+ reversed_order: list[OrderTuple] = list(query.order)
+ reversed_order[order_index] = OrderTerm(
+ member,
+ term[1],
+ NullPlacement.FIRST if placement is NullPlacement.LAST else
NullPlacement.LAST,
+ )
+ reversed_page: pa.Table = view.get_table(
+ replace(query, order=reversed_order)
+ ).results
+ # Compare every declared key; payload order inside a complete key tie is
free.
+ key_pages: list[pa.Table] = [
+ reversed_page.select(order_names),
+ full.take(opposite).slice(start, query.limit).select(order_names),
Review Comment:
Nit: With an encoded-NULL dictionary key split across chunks,
`full.take(opposite)` raises `ArrowInvalid: Cannot yet unify dictionaries with
nulls` before the cast below runs, so a correct provider still fails. Decoding
the key columns before `take` avoids it.
```suggestion
# Decode before ``take``: it cannot unify chunk dictionaries with
NULLs.
pa.Table.from_arrays(
[
c.cast(c.type.value_type) if pa.types.is_dictionary(c.type)
else c
for c in full.select(order_names).columns
],
order_names,
)
.take(opposite)
.slice(start, query.limit),
```
--
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]
