From 6b0fd79b55ad6bc76048e08d767a36815fd0ea93 Mon Sep 17 00:00:00 2001 From: Hussain Sultan Date: Wed, 12 Aug 2026 11:46:01 -0400 Subject: [PATCH] fix: dtype-blind date coercion, silent field drop on tagging, id()-based names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects found by probing invariants against the current branch. JSON filters coerced any complete-ISO string to a date literal without looking at the column, so a string column holding ISO-looking text (a "2024-01-01" batch label) could not be filtered at all — the comparison was rebuilt as string-vs-timestamp and the backend rejected it. Coercion now requires a temporal column, and like/ilike never coerce since their right operand is a string pattern. The coercion that Athena-style backends need is unchanged, and an unknown dtype still coerces: only a positively non-temporal column suppresses it. Tagging dropped a whole field set when one field failed to serialize. `value_or({})` turned "this measure holds a Python set" into `measures: ()`, so the model tagged fine, reconstructed with no measures at all, and only failed later as "Column 'total' is not found" — blaming the query rather than the field. Same for dimensions, and calc measures were skipped one at a time in both directions. All of these now name the field that could not be written. This mattered more after the previous commit's callable allowlist, which makes more expressions unserializable. Positional measures were named `_measure_{id(item)}`, so the result column was a memory address: it changed between runs and it reached the xorq tag, making the metadata for one query differ per process. Names are now positional (`_measure_0`), skipping any name a keyword measure already claims — a collision the id() scheme was accidentally immune to. Note on the cache key: a plain xorq memtable already hashes differently across processes with no BSL involvement, so this fix removes BSL's contribution to tag instability but does not by itself make memtable expressions cacheable. 1292 tests pass; 15 added. Co-Authored-By: Claude Opus 5 --- src/boring_semantic_layer/expr.py | 23 ++- src/boring_semantic_layer/predicate.py | 41 +++- .../serialization/extract.py | 52 ++++- .../tests/test_round7_defects.py | 191 ++++++++++++++++++ 4 files changed, 292 insertions(+), 15 deletions(-) create mode 100644 src/boring_semantic_layer/tests/test_round7_defects.py 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)