diff --git a/src/boring_semantic_layer/expr.py b/src/boring_semantic_layer/expr.py index a1d5c5b1..14cde2c5 100644 --- a/src/boring_semantic_layer/expr.py +++ b/src/boring_semantic_layer/expr.py @@ -1935,18 +1935,35 @@ def aggregate( **aliased, ): aggs = {} - for item in measure_names: + + def anonymous_name(position: int) -> str: + """Name a positional measure by where it appears, not by ``id()``. + + ``_measure_{id(item)}`` derived the result column from a memory + address: the column name changed between runs, and because the + name reaches the xorq tag it changed the expression hash too, so + an identical query never hit the cache. + """ + candidate = f"_measure_{position}" + # ``aliased`` is merged in after this loop, so a keyword measure + # occupying the positional name would otherwise overwrite it. + while candidate in aggs or candidate in aliased: + position += 1 + candidate = f"_measure_{position}" + return candidate + + for index, item in enumerate(measure_names): if _is_deferred(item): try: name = _normalize_to_name(item) aggs[name] = make_bare_ref_lambda(name) except TypeError: # Complex Deferred (e.g. _.distance.sum()) — treat as callable - aggs[f"_measure_{id(item)}"] = item + aggs[anonymous_name(index)] = item elif isinstance(item, str): aggs[item] = make_bare_ref_lambda(item) elif callable(item): - aggs[f"_measure_{id(item)}"] = item + aggs[anonymous_name(index)] = item else: raise TypeError( f"measure_names must be strings, callables, or Deferred expressions, " diff --git a/src/boring_semantic_layer/predicate.py b/src/boring_semantic_layer/predicate.py index d5b9f40f..33fb2955 100644 --- a/src/boring_semantic_layer/predicate.py +++ b/src/boring_semantic_layer/predicate.py @@ -37,6 +37,9 @@ "not_ilike": lambda x, y: ~x.ilike(y), } +#: Comparisons whose right operand is a string pattern, never a literal date. +_PATTERN_OPS = frozenset({"like", "not_like", "ilike", "not_ilike"}) + # JSON filter operator strings that map to a Compare node. Includes # legacy aliases (``=``, ``equals``) accepted by the existing parser. _DICT_COMPARE_OPS: dict[str, str] = { @@ -208,7 +211,7 @@ def _reject_value_keys(spec: dict, op: str) -> None: raise ValueError(f"Operator {op!r} should not have 'value' or 'values' fields") -def _convert_literal(value: Any, ibis_module) -> Any: +def _convert_literal(value: Any, ibis_module, column: Any = None) -> Any: """Convert complete ISO date/timestamp strings to typed ibis literals. Backends like Athena require typed date literals or fail with @@ -217,9 +220,16 @@ def _convert_literal(value: Any, ibis_module) -> Any: or "12:30" with *today's* date, so coercing them would make results depend on the day the query runs. Other strings pass through unchanged. + + Coercion also depends on *column*: a string column that happens to hold + ISO-looking text ("2024-01-01" as a batch label) is compared as text. + Without this check the comparison was rebuilt as string-vs-timestamp and + the backend rejected a perfectly valid filter. """ if not isinstance(value, str) or not _is_complete_iso_datetime(value): return value + if column is not None and not _is_temporal_column(column): + return value for dtype in ("timestamp", "date"): try: return ibis_module.literal(value, type=dtype) @@ -228,6 +238,25 @@ def _convert_literal(value: Any, ibis_module) -> Any: return value +def _is_temporal_column(column: Any) -> bool: + """True when *column* holds dates/times, so a date literal is comparable. + + Unknown dtypes answer True to preserve the coercion that backends like + Athena need; only a positively non-temporal column suppresses it. + """ + try: + dtype = column.type() + except Exception: + return True + for probe in ("is_temporal", "is_timestamp", "is_date", "is_time"): + check = getattr(dtype, probe, None) + if callable(check) and check(): + return True + return not any( + callable(getattr(dtype, probe, None)) for probe in ("is_temporal", "is_string") + ) + + def _is_complete_iso_datetime(value: str) -> bool: for parse in (datetime.date.fromisoformat, datetime.datetime.fromisoformat): try: @@ -349,7 +378,7 @@ def compile( # noqa: A001 post_agg=post_agg, strict_qualified=strict_qualified, ) - values = [_convert_literal(v, ibis_module) for v in pred.values] + values = [_convert_literal(v, ibis_module, col) for v in pred.values] return col.notin(values) if pred.negate else col.isin(values) if isinstance(pred, Compare): col = _field_accessor( @@ -358,7 +387,13 @@ def compile( # noqa: A001 post_agg=post_agg, strict_qualified=strict_qualified, ) - value = _convert_literal(pred.value, ibis_module) + # like/ilike are string pattern matches: a date literal can never be + # the right operand, whatever the column's type. + value = ( + pred.value + if pred.op in _PATTERN_OPS + else _convert_literal(pred.value, ibis_module, col) + ) return _COMPARE_OPS[pred.op](col, value) if isinstance(pred, Custom): return pred.fn(table) diff --git a/src/boring_semantic_layer/serialization/extract.py b/src/boring_semantic_layer/serialization/extract.py index 9016ee83..4e1698b5 100644 --- a/src/boring_semantic_layer/serialization/extract.py +++ b/src/boring_semantic_layer/serialization/extract.py @@ -85,16 +85,38 @@ def _ensure_registered(): # --------------------------------------------------------------------------- +def _unwrap_or_raise(result: Result[dict, Exception], kind: str, model_name) -> dict: + """Return the serialized fields, or explain which kind could not be written. + + ``value_or({})`` here meant that one unserializable field emptied the + whole set: a model with a measure holding, say, a Python ``set`` was + tagged with ``measures: ()``, reconstructed with no measures at all, and + only failed later as "Column 'total' is not found" — pointing at the + query rather than at the field that could not be serialized. + """ + if isinstance(result, Success): + return result.unwrap() + where = f" on model {model_name!r}" if model_name else "" + raise ValueError( + f"Cannot serialize the {kind}{where}: {result.failure()}. Tagging would " + f"otherwise drop every {kind} silently and the reconstructed model would " + "be missing them." + ) + + @_register_lazy("SemanticTableOp") def _extract_semantic_table(op, context: BSLSerializationContext) -> dict[str, Any]: - dims_result = serialize_dimensions(op.get_dimensions()) - meas_result = serialize_measures(op.get_measures()) - calc_result = serialize_calc_measures(op.get_calculated_measures()) metadata: dict[str, Any] = { - "dimensions": dims_result.value_or({}), - "measures": meas_result.value_or({}), + "dimensions": _unwrap_or_raise( + serialize_dimensions(op.get_dimensions()), "dimensions", op.name + ), + "measures": _unwrap_or_raise( + serialize_measures(op.get_measures()), "measures", op.name + ), } - calc_data = calc_result.value_or({}) + calc_data = _unwrap_or_raise( + serialize_calc_measures(op.get_calculated_measures()), "calculated measures", op.name + ) if calc_data: metadata["calc_measures"] = calc_data if op.name: @@ -348,7 +370,12 @@ def do_serialize(): case Success(): entry["expr_struct"] = struct_result.unwrap() case _: - continue + # Skipping left the model looking complete while quietly + # missing this calc measure. + raise ValueError( + f"Calc measure {name!r}: failed to serialize expression " + f"({struct_result.failure()})" + ) description = getattr(calc, "description", None) if description is not None: entry["description"] = description @@ -393,7 +420,11 @@ def deserialize_calc_measures(calc_data: Mapping[str, Any]) -> dict[str, Any]: depends_on = frozenset() if struct is None: - continue + raise ValueError( + f"Calc measure {name!r} has no serialized expression in this " + "payload; reconstructing without it would silently return a " + "model that is missing the measure." + ) # ``thaw`` converts the resolver tuple into a list of lists; the # resolver deserializer expects nested tuples, so convert back. struct = list_to_tuple(struct) @@ -402,7 +433,10 @@ def deserialize_calc_measures(calc_data: Mapping[str, Any]) -> dict[str, Any]: case Success(): expr = result.unwrap() case _: - continue + raise ValueError( + f"Calc measure {name!r}: failed to deserialize expression " + f"({result.failure()})" + ) out[name] = CalcMeasure( expr=expr, description=description, diff --git a/src/boring_semantic_layer/tests/test_round7_defects.py b/src/boring_semantic_layer/tests/test_round7_defects.py new file mode 100644 index 00000000..8f3374b3 --- /dev/null +++ b/src/boring_semantic_layer/tests/test_round7_defects.py @@ -0,0 +1,191 @@ +"""Regression tests for a third round of defects. + +Three independent problems, all of which either rejected a valid query or +quietly returned a model that was missing part of itself: + +1. JSON filters coerced any complete-ISO string to a date literal without + looking at the column, so a string column holding ISO-looking text could + not be filtered at all. +2. One unserializable field emptied its whole field set during tagging, so + the reconstructed model silently lost every measure (or every dimension). +3. Positional measures were named from ``id()``, making the result column + name — and the tag metadata — differ between runs of the same query. +""" + +from __future__ import annotations + +import datetime + +import ibis +import pytest + +from boring_semantic_layer import to_semantic_table +from boring_semantic_layer.serialization import from_tagged, to_tagged + + +@pytest.fixture +def sales(): + return ibis.memtable( + { + "region": ["west", "west", "east", "east", "east"], + "rep": ["a", "b", "c", "d", "e"], + "amount": [10, 20, 30, 40, 50], + # ISO-looking *text*, not a date column + "batch": ["2024-01-01", "2024-01-01", "2024-06-01", "2024-06-01", "2025-01-01"], + "ts": [ + datetime.datetime(2024, 1, 15), + datetime.datetime(2024, 2, 15), + datetime.datetime(2024, 6, 15), + datetime.datetime(2024, 7, 15), + datetime.datetime(2025, 1, 15), + ], + } + ) + + +@pytest.fixture +def model(sales): + return ( + to_semantic_table(sales, "sales") + .with_dimensions( + region=lambda t: t.region, + batch=lambda t: t.batch, + ts=lambda t: t.ts, + ) + .with_measures(total=lambda t: t.amount.sum()) + ) + + +def _total(model, *filters): + df = model.query(dimensions=[], measures=["total"], filters=list(filters)).execute() + return float(df["total"][0]) + + +# --------------------------------------------------------------------------- +# 1. Date-literal coercion must respect the column's type +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "spec,expected", + [ + ({"field": "batch", "operator": "=", "value": "2024-01-01"}, 30.0), + ({"field": "batch", "operator": "!=", "value": "2024-01-01"}, 120.0), + ( + {"field": "batch", "operator": "in", "values": ["2024-01-01", "2025-01-01"]}, + 80.0, + ), + ], +) +def test_iso_looking_text_filters_a_string_column(model, spec, expected): + """A string column holding "2024-01-01" is compared as text. + + Coercing to a timestamp made the backend reject the comparison outright + (``batch:string and Literal(...):timestamp are not comparable``). + """ + assert _total(model, spec) == expected + + +@pytest.mark.parametrize( + "value,expected", + [("2024-01-01", 30.0), ("2024-%", 100.0), ("%-06-01", 70.0)], +) +def test_like_patterns_are_never_coerced(model, value, expected): + """like/ilike take a string pattern, whatever the column's type.""" + assert _total(model, {"field": "batch", "operator": "like", "value": value}) == expected + + +def test_temporal_columns_still_get_typed_literals(model): + """The coercion exists for backends that need typed dates — keep it.""" + assert _total(model, {"field": "ts", "operator": ">=", "value": "2024-06-01"}) == 120.0 + + +def test_json_filter_matches_the_equivalent_lambda(model): + json_filtered = _total(model, {"field": "region", "operator": "=", "value": "east"}) + lambda_filtered = float( + model.filter(lambda t: t.region == "east").aggregate("total").execute()["total"][0] + ) + assert json_filtered == lambda_filtered == 120.0 + + +# --------------------------------------------------------------------------- +# 2. A field that cannot be serialized must not take the others with it +# --------------------------------------------------------------------------- + + +def test_unserializable_measure_fails_loudly(sales): + """``value_or({})`` dropped *every* measure when one could not be written. + + The model then reconstructed with no measures and failed at query time + with "Column 'total' is not found", blaming the query. + """ + model = ( + to_semantic_table(sales, "m") + .with_dimensions(region=lambda t: t.region) + .with_measures( + total=lambda t: t.amount.sum(), + # a set is unhashable, so the resolver tree cannot hold it + bad=lambda t: t.rep.isin({"a", "b"}).sum(), + ) + ) + with pytest.raises(ValueError, match="measures"): + to_tagged(model) + + +def test_unserializable_dimension_fails_loudly(sales): + model = ( + to_semantic_table(sales, "m") + .with_dimensions(region=lambda t: t.region, bad=lambda t: t.rep.isin({"a", "b"})) + .with_measures(total=lambda t: t.amount.sum()) + ) + with pytest.raises(ValueError, match="dimensions"): + to_tagged(model) + + +def test_serializable_model_still_round_trips_every_field(model): + """The guard must not cost a well-formed model anything.""" + restored = from_tagged(to_tagged(model)) + assert set(restored.op().get_measures()) == {"total"} + assert set(restored.op().get_dimensions()) == {"region", "batch", "ts"} + assert float(restored.aggregate("total").execute()["total"][0]) == 150.0 + + +# --------------------------------------------------------------------------- +# 3. Positional measures need a deterministic name +# --------------------------------------------------------------------------- + + +def test_positional_measure_name_is_positional(model): + """``_measure_{id(item)}`` made the result column a memory address.""" + df = model.group_by("region").aggregate(lambda t: t.amount.sum()).execute() + assert [c for c in df.columns if c != "region"] == ["_measure_0"] + + +def test_several_positional_measures_get_distinct_stable_names(model): + df = ( + model.group_by("region") + .aggregate(lambda t: t.amount.sum(), lambda t: t.amount.max()) + .execute() + ) + assert sorted(c for c in df.columns if c != "region") == ["_measure_0", "_measure_1"] + + +def test_positional_measure_name_does_not_collide_with_an_alias(model): + """A user alias occupying the positional name must not be overwritten.""" + df = ( + model.group_by("region") + .aggregate(lambda t: t.amount.sum(), _measure_0=lambda t: t.amount.max()) + .execute() + ) + cols = sorted(c for c in df.columns if c != "region") + assert cols == ["_measure_0", "_measure_1"] + west = df[df["region"] == "west"].iloc[0] + assert {float(west["_measure_0"]), float(west["_measure_1"])} == {20.0, 30.0} + + +def test_tag_metadata_is_identical_for_the_same_query(model): + """The name reaches the xorq tag, so an address made metadata unstable.""" + first = dict(to_tagged(model.group_by("region").aggregate(lambda t: t.amount.sum())).op().metadata) + second = dict(to_tagged(model.group_by("region").aggregate(lambda t: t.amount.sum())).op().metadata) + assert repr(first) == repr(second) + assert "_measure_0" in repr(first)