aminghadersohi commented on code in PR #43777:
URL: https://github.com/apache/superset/pull/43777#discussion_r3926701501
##########
superset/semantic_layers/models.py:
##########
@@ -322,6 +368,121 @@ def get_query_result(self, query_object: QueryObject) ->
QueryResult:
def get_query_str(self, query_obj: QueryObjectDict) -> str:
return "Not implemented for semantic layers"
+ @property
+ def normalize_columns(self) -> bool:
+ """Dimension names are provider-verbatim; nothing to (de)normalize.
+
+ Exists for the datasource values endpoint, which reads it before
+ requesting filter-value suggestions.
+ """
+ return False
+
+ def values_for_column(
+ self,
+ column_name: str,
+ limit: int = 10000,
+ denormalize_column: bool = False, # pylint: disable=unused-argument
+ array_elements: bool = False, # pylint: disable=unused-argument
+ search: str | None = None,
+ ) -> list[Any]:
+ """Return the distinct values of one dimension for filter suggestions.
+
+ Delegates to the provider ABC's purpose-built ``get_values`` — an
+ abstract member every provider implements, consumed here for the
+ first time. ``search`` narrows at the provider with a containment
+ ``LIKE`` filter on the dimension, so values beyond the first page are
+ findable. Two documented parity gaps with datasets, both inherent to
+ the standard filter model: case sensitivity follows the provider's
+ collation (no case-folding operator), and ``%``/``_`` in the search
+ term act as wildcards (no portable escape declaration; over-matching
+ is the safe failure for suggestions). A provider that rejects the
+ narrowing filter — a non-text dimension, say — falls back to the
+ unfiltered bounded page with a logged warning rather than an error.
+
+ ``get_values`` takes no limit or order, so both are applied here:
+ sorted ascending (nulls first) and then truncated, so the page is
+ deterministic and not an arbitrary provider-order subset.
+ ``denormalize_column`` and ``array_elements`` are dataset concepts
+ (dialect name denormalization, array-element explosion) with no
+ semantic-view counterpart; they are accepted for endpoint signature
+ compatibility and ignored.
+
+ Raises ``KeyError`` for a name that is not a dimension of the view —
+ a metric name included — which the endpoint reports as the caller's
+ error naming the column, exactly as a dataset does.
+ """
+ dimensions = {
+ dimension.name: dimension for dimension in self._unique_dimensions
+ }
+ if column_name not in dimensions:
+ raise KeyError(column_name)
+ dimension = dimensions[column_name]
+
+ if search:
+ narrowing = Filter(
+ type=PredicateType.WHERE,
+ column=dimension,
+ operator=Operator.LIKE,
+ value=f"%{search}%",
+ )
+ try:
+ result = self.implementation.get_values(dimension, {narrowing})
+ except Exception: # pylint: disable=broad-exception-caught
+ # The narrowing filter is best-effort: a provider that cannot
+ # apply it must degrade to the bounded first page (the picker
+ # still narrows within it), never to an error — but say so,
+ # or the degradation is the next silent failure.
+ logger.warning(
+ "Semantic view %s rejected the value-search filter on "
+ "dimension %s; returning the unfiltered page",
+ self.uuid,
+ dimension.name,
+ exc_info=True,
+ )
+ result = self.implementation.get_values(dimension, None)
+ else:
+ result = self.implementation.get_values(dimension, None)
+
+ # Some drivers report zero rows as ``results is None``.
+ if result.results is None or result.results.num_rows == 0:
+ return []
+ table = stringify_extension_columns(result.results)
+ if dimension.name in table.column_names:
+ column = table.column(dimension.name)
+ elif table.num_columns == 1:
+ column = table.column(0)
+ else:
+ # A provider-contract violation is the server's fault, not the
+ # caller's; surface it rather than mislabeling it a bad column.
+ raise ValueError(
+ f"Provider result is missing the requested dimension
{dimension.name}"
+ )
+ # Non-finite floats must not leave the model: NaN/Infinity render the
+ # endpoint's body as invalid strict JSON (the browser's JSON.parse
+ # throws and the picker silently empties, with nothing in the server
+ # log), and NaN defeats the ascending sort (every comparison is
+ # False). Collapse them to None -- the same outcome the dataset path
+ # produces by replacing NaN after the query.
+ values = [
+ None if isinstance(value, float) and not math.isfinite(value) else
value
+ for value in column.to_pylist()
+ ]
Review Comment:
The `isinstance(value, float)` test only sees the top level, so a non-finite
float nested inside a STRUCT or LIST dimension passes through untouched and
lands in the response body raw. Measured through the endpoint at this head:
```
struct<f:float64> 200 {"result":[{"f":1.0},{"f":NaN}]} -> invalid strict
JSON
list<float64> 200 {"result":[[NaN],[1.0]]} -> invalid strict
JSON
```
Same failure this commit fixes for scalars: `JSON.parse` throws, the
popover's `catch` returns `{ data: [], totalCount: 0 }`, empty picker, nothing
in the server log. The ordering half leaks too — `[[NaN],[1.0]]` came back
unsorted, because floats inside lists are comparable enough not to raise while
`NaN` still compares `False` against everything.
This is narrow, and it's only reachable *because* `4ca4d8a` made struct/list
dimensions sort correctly — before that they raised earlier. A struct with a
float field or a `list<float64>` (scores, embeddings, a lat/lon pair) is the
realistic shape.
Recursing fixes it. I ran this against `struct<f>`, `list<f64>`,
`struct<struct<f>>`, `list<struct<f>>` and the scalar and string-struct
controls — all strict-JSON valid, scalar ordering unchanged, and the
canonical-string fallback below still sorts the results:
```python
def _json_safe(value: Any) -> Any:
"""Collapse non-finite floats to None at any nesting depth."""
if isinstance(value, float):
return value if math.isfinite(value) else None
if isinstance(value, dict):
return {key: _json_safe(item) for key, item in value.items()}
if isinstance(value, (list, tuple)):
return [_json_safe(item) for item in value]
return value
values = [_json_safe(value) for value in column.to_pylist()]
```
Worth a case in `test_values_for_column_normalizes_non_finite_floats` with a
struct- or list-typed column so the nesting is pinned, since the current test
is scalar-only.
##########
tests/unit_tests/semantic_layers/models_test.py:
##########
@@ -1609,3 +1613,397 @@ def
test_build_semantic_view_query_no_perm_excludes(app: Any) -> None:
assert view.id not in item_ids
finally:
db.session.rollback()
+
+
+def _values_result(values: list[Any], name: str = "category") ->
SemanticResult:
+ return SemanticResult(
+ requests=[SemanticRequest(type="SQL", definition="values query")],
+ results=pa.table({name: pa.array(values)}),
+ )
+
+
+def test_values_for_column_delegates_to_get_values_sorted_and_limited(
+ mock_implementation: MagicMock,
+ mock_dimensions: list[Dimension],
+) -> None:
+ """The provider ABC's purpose-built get_values is the fetch; the host
+ sorts ascending and truncates, so the page is deterministic rather than
+ an arbitrary provider-order subset."""
+ view = SemanticView()
+ mock_implementation.get_values.return_value = _values_result(
+ ["Electronics", "Books", "Clothing"]
+ )
+
+ with patch.object(
+ SemanticView,
+ "implementation",
+ new_callable=lambda: property(lambda s: mock_implementation),
+ ):
+ assert view.values_for_column("category", limit=2) == ["Books",
"Clothing"]
+
+ mock_implementation.get_values.assert_called_once_with(mock_dimensions[1],
None)
+ mock_implementation.get_table.assert_not_called()
+
+
+def test_values_for_column_dataset_endpoint_flags_are_ignored(
+ mock_implementation: MagicMock,
+) -> None:
+ """denormalize_column/array_elements are accepted for endpoint signature
+ compatibility and change nothing."""
+ view = SemanticView()
+ mock_implementation.get_values.return_value = _values_result(["x"])
+
+ with patch.object(
+ SemanticView,
+ "implementation",
+ new_callable=lambda: property(lambda s: mock_implementation),
+ ):
+ assert view.values_for_column(
+ "category", denormalize_column=True, array_elements=True
+ ) == ["x"]
+
+
+def test_values_for_column_unknown_column_and_metric_raise_key_error(
+ mock_implementation: MagicMock,
+) -> None:
+ """Unknown names — metric names included — are the caller's error; the
+ endpoint maps KeyError to a 400 naming the column."""
+ view = SemanticView()
+
+ with patch.object(
+ SemanticView,
+ "implementation",
+ new_callable=lambda: property(lambda s: mock_implementation),
+ ):
+ with pytest.raises(KeyError):
+ view.values_for_column("no_such_column")
+ with pytest.raises(KeyError):
+ view.values_for_column("revenue")
+ mock_implementation.get_values.assert_not_called()
+
+
+def test_values_for_column_empty_and_none_results_return_empty_list(
+ mock_implementation: MagicMock,
+) -> None:
+ view = SemanticView()
+
+ with patch.object(
+ SemanticView,
+ "implementation",
+ new_callable=lambda: property(lambda s: mock_implementation),
+ ):
+ mock_implementation.get_values.return_value = _values_result([])
+ assert view.values_for_column("category") == []
+
+ mock_implementation.get_values.return_value = SemanticResult(
+ requests=[SemanticRequest(type="SQL", definition="values query")],
+ results=None,
+ )
+ assert view.values_for_column("category") == []
+
+
+def test_values_for_column_nulls_sort_first_and_numbers_survive(
+ mock_implementation: MagicMock,
+) -> None:
+ """Non-text values arrive JSON-safe and typed; arrow nulls become None
+ and sort ahead of values."""
+ view = SemanticView()
+ mock_implementation.get_values.return_value = SemanticResult(
+ requests=[SemanticRequest(type="SQL", definition="values query")],
+ results=pa.table({"category": pa.array([3.0, None, 1.5])}),
+ )
+
+ with patch.object(
+ SemanticView,
+ "implementation",
+ new_callable=lambda: property(lambda s: mock_implementation),
+ ):
+ assert view.values_for_column("category") == [None, 1.5, 3.0]
+
+
+def test_values_for_column_single_unnamed_column_is_accepted(
+ mock_implementation: MagicMock,
+) -> None:
+ """get_values contracts a single-column table; a provider that names the
+ column differently still works when there is exactly one column."""
+ view = SemanticView()
+ mock_implementation.get_values.return_value = _values_result(["x"],
name="anything")
+
+ with patch.object(
+ SemanticView,
+ "implementation",
+ new_callable=lambda: property(lambda s: mock_implementation),
+ ):
+ assert view.values_for_column("category") == ["x"]
+
+
+def test_values_for_column_ambiguous_result_is_a_server_error(
+ mock_implementation: MagicMock,
+) -> None:
+ """A multi-column result without the dimension is not the caller's 400."""
+ view = SemanticView()
+ mock_implementation.get_values.return_value = SemanticResult(
+ requests=[SemanticRequest(type="SQL", definition="values query")],
+ results=pa.table({"a": pa.array(["x"]), "b": pa.array(["y"])}),
+ )
+
+ with patch.object(
+ SemanticView,
+ "implementation",
+ new_callable=lambda: property(lambda s: mock_implementation),
+ ):
+ with pytest.raises(ValueError, match="category"):
+ view.values_for_column("category")
+
+
[email protected]("reverse", [False, True])
+def test_values_for_column_uses_grain_collapsed_dimensions(
+ mock_implementation: MagicMock,
+ mock_dimensions: list[Dimension],
+ reverse: bool,
+) -> None:
+ """Grain variants share a name; the values fetch uses the same collapsed
+ dimension that columns/column_names present to the picker, and the pick
+ must not depend on ``get_dimensions()`` iteration order — the ABC returns
+ a set. The least-aggregated variant wins: DAY-truncated values beat
+ MONTH-truncated ones as suggestions. Both orders assert the same pick."""
+ variant = Dimension(
+ id="orders.order_date",
+ name="order_date",
+ type=pa.date32(),
+ definition="orders.order_date",
+ grain=Grains.MONTH,
+ )
+ dims = [*mock_dimensions, variant]
+ if reverse:
+ dims = list(reversed(dims))
+ mock_implementation.get_dimensions.return_value = dims
+ mock_implementation.get_values.return_value = _values_result([],
name="order_date")
+ view = SemanticView()
+
+ with patch.object(
+ SemanticView,
+ "implementation",
+ new_callable=lambda: property(lambda s: mock_implementation),
+ ):
+ view.values_for_column("order_date")
+
+ assert mock_implementation.get_values.call_args.args[0] ==
mock_dimensions[0]
+
+
[email protected]("reverse", [False, True])
+def test_values_for_column_prefers_the_unaggregated_variant(
+ mock_implementation: MagicMock,
+ mock_dimensions: list[Dimension],
+ reverse: bool,
+) -> None:
+ """When a name has an unaggregated variant (``grain is None``) alongside
+ grained ones, the unaggregated one is the suggestion source, whatever
+ order the provider's set iterates in."""
+ unaggregated = Dimension(
+ id="orders.order_date",
+ name="order_date",
+ type=pa.date32(),
+ definition="orders.order_date",
+ grain=None,
+ )
+ monthly = Dimension(
+ id="orders.order_date",
+ name="order_date",
+ type=pa.date32(),
+ definition="orders.order_date",
+ grain=Grains.MONTH,
+ )
+ dims = [*mock_dimensions, monthly, unaggregated]
+ if reverse:
+ dims = list(reversed(dims))
+ mock_implementation.get_dimensions.return_value = dims
+ mock_implementation.get_values.return_value = _values_result([],
name="order_date")
+ view = SemanticView()
+
+ with patch.object(
+ SemanticView,
+ "implementation",
+ new_callable=lambda: property(lambda s: mock_implementation),
+ ):
+ view.values_for_column("order_date")
+
+ assert mock_implementation.get_values.call_args.args[0] == unaggregated
+
+
+def test_values_for_column_sorts_struct_values_without_error(
+ mock_implementation: MagicMock,
+ mock_dimensions: list[Dimension],
+) -> None:
+ """A STRUCT-typed dimension arrives as Python dicts, which have no natural
+ order; the sort must fall back to a deterministic canonical order instead
+ of raising TypeError, keeping nulls first."""
+ view = SemanticView()
+ mock_implementation.get_values.return_value = SemanticResult(
+ requests=[SemanticRequest(type="SQL", definition="values query")],
+ results=pa.table(
+ {
+ "category": pa.array(
+ [{"code": "b", "n": 2}, None, {"code": "a", "n": 1}],
+ type=pa.struct([("code", pa.string()), ("n", pa.int64())]),
+ )
+ }
+ ),
+ )
+
+ with patch.object(
+ SemanticView,
+ "implementation",
+ new_callable=lambda: property(lambda s: mock_implementation),
+ ):
+ values = view.values_for_column("category")
+
+ # Exact expected order: nulls first, then canonical-string order of
+ # the dicts (json.dumps with sorted keys puts code "a" before "b").
+ assert values == [None, {"code": "a", "n": 1}, {"code": "b", "n": 2}]
+
+
+def test_values_for_column_sorts_list_values_with_null_elements(
+ mock_implementation: MagicMock,
+ mock_dimensions: list[Dimension],
+) -> None:
+ """A LIST-typed dimension can hold null ELEMENTS inside the arrays;
+ comparing [None, "a"] with ["a"] raises TypeError under natural ordering,
+ so the fallback order must apply. Whole-null values still sort first."""
+ view = SemanticView()
+ mock_implementation.get_values.return_value = SemanticResult(
+ requests=[SemanticRequest(type="SQL", definition="values query")],
+ results=pa.table(
+ {
+ "category": pa.array(
+ [[None, "a"], None, ["a"], ["b", None]],
+ type=pa.list_(pa.string()),
+ )
+ }
+ ),
+ )
+
+ with patch.object(
+ SemanticView,
+ "implementation",
+ new_callable=lambda: property(lambda s: mock_implementation),
+ ):
+ values = view.values_for_column("category")
+
+ assert values[0] is None
+ assert len(values) == 4
+ assert [None, "a"] in values and ["a"] in values and ["b", None] in values
Review Comment:
`pre-commit` is failing at this head on this line — ruff `PT018`:
```
tests/unit_tests/semantic_layers/models_test.py:1895:5: PT018 Assertion
should be broken down into multiple parts
1895 | assert [None, "a"] in values and ["a"] in values and ["b",
None] in values
```
Run history: green at `81bf2b00`, failure from `4ca4d8a8` onward, still
failing at `fca1de9e` (run `33774659274`). No autofix available — ruff reports
the fix as unsafe-only. Splitting it clears the hook and gives a better failure
message when it does break:
```python
assert [None, "a"] in values
assert ["a"] in values
assert ["b", None] in values
```
Every other check on the commit is green: 45 success, 9 skipped, 3 neutral,
0 cancelled, and the one StatusContext (`netlify/superset-docs-preview`) is
success. This is the only red leg.
--
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]
