mikebridge commented on code in PR #45912:
URL: https://github.com/apache/superset/pull/45912#discussion_r4461641383
##########
superset-core/src/superset_core/semantic_layers/testing.py:
##########
@@ -272,6 +299,100 @@ 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[object] = full.column(member.name).to_pylist()
Review Comment:
Fixed in cfbcecbf17, after merging master. The witness checks each Arrow
scalar’s validity instead of converting the column to Python values. Added
timestamp[ns] and time64[ns] regressions with values 1 and 2 ns in a subprocess
that makes pandas unavailable. Both fail on the original expression and pass
with this change.
The complete PR test set passes: 2,030 tests, with 4 capability skips;
conformance self-tests and hooks also pass. The fix does not change the public
conformance API.
##########
superset-core/src/superset_core/semantic_layers/testing.py:
##########
@@ -272,6 +299,100 @@ 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[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 order_names)
+ for index in range(full.num_rows)
+ ]
Review Comment:
Fixed in cfbcecbf17. Order-key projection now unwraps DictionaryScalar.value
before grouping or comparing pages. The regression uses different dictionaries
in separate chunks for (one,A), (one,NULL), (two,A): the encoded scalars
differ, but the decoded one values form a single prefix tie. It was rejected
before the fix and passes now, including the reversed-placement page check.
Master’s empty-membership cases were retained in the merge. The full PR test
set passes: 2,030 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]
