From 1de4f0cba3d0053254ca051d3336474e191e5fe8 Mon Sep 17 00:00:00 2001 From: Hussain Sultan Date: Sun, 23 Aug 2026 22:24:14 -0400 Subject: [PATCH] fix: integer totals columns truncated t.all() ratios to 0 on DataFusion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit agg.mutate(share=lambda t: t.total / t.all(t.total)) routes t.all through attach_windowed_totals, whose __bsl_totals__ column kept the measure's integer dtype. xorq's DataFusion executes ibis truediv on two int64 operands as integer division, so every share came back 0.0 on the memtable → canonical-backend path — while the identical calc-measure spelling was correct (its path already casts via _float_total) and the duckdb-compiled SQL carried the cast. Apply the same integer→float64 policy where the totals columns are created: attach_windowed_totals casts the windowed total, and attach_calc_totals casts calc-of-calc totals, so an int-valued calc cannot reintroduce the truncation downstream. test_totals_semantics.py drops the duckdb pin (the inline spelling now asserts SUM_SHARE on the shared memtable fixture) and gains a dedicated regression test covering integer sum and count measures. Co-Authored-By: Claude Fable 5 --- src/boring_semantic_layer/calc_compiler.py | 12 +++- .../tests/test_totals_semantics.py | 55 +++++++++++++------ 2 files changed, 48 insertions(+), 19 deletions(-) diff --git a/src/boring_semantic_layer/calc_compiler.py b/src/boring_semantic_layer/calc_compiler.py index 113f2195..7f4f478b 100644 --- a/src/boring_semantic_layer/calc_compiler.py +++ b/src/boring_semantic_layer/calc_compiler.py @@ -48,7 +48,7 @@ ) from .errors import suggest_kinded from .fieldref import resolve_suffix -from .measure_scope import UnknownMeasureRefError +from .measure_scope import UnknownMeasureRefError, _float_total logger = logging.getLogger(__name__) @@ -692,7 +692,11 @@ def attach_windowed_totals( "substitute the per-group value for the grand total." ) from exc col = f"{totals_prefix}{name}" - new_base = new_base.mutate(**{col: windowed}) + # Integral totals become float64 so ``measure / t.all(measure)`` is + # a true ratio: with two integer operands some engines (xorq's + # DataFusion among them) do integer division, and ``60 / 160`` + # silently came back as 0. Same policy as ``MeasureScope.all``. + new_base = new_base.mutate(**{col: _float_total(windowed)}) arbitrary_specs[col] = lambda t, _c=col: t[_c].arbitrary() return new_base, arbitrary_specs @@ -815,7 +819,9 @@ def attach_calc_totals( "per-group value for the grand total." ) from exc col = f"{totals_prefix}{calc_name}" - real_agg_tbl = real_agg_tbl.mutate(**{col: totals_expr}) + # Same integer→float64 policy as attach_windowed_totals: an + # int-valued calc's totals column must not integer-divide later. + real_agg_tbl = real_agg_tbl.mutate(**{col: _float_total(totals_expr)}) return real_agg_tbl diff --git a/src/boring_semantic_layer/tests/test_totals_semantics.py b/src/boring_semantic_layer/tests/test_totals_semantics.py index 10a10594..a778f09d 100644 --- a/src/boring_semantic_layer/tests/test_totals_semantics.py +++ b/src/boring_semantic_layer/tests/test_totals_semantics.py @@ -98,6 +98,40 @@ def test_integer_measure_over_integer_total_is_a_ratio(model): assert _shares(model.with_measures(share=lambda t: t.total / t.all(t.total))) == SUM_SHARE +def test_integer_ratio_via_aggregate_mutate_is_a_ratio(model): + """The inline spelling must match the declared one for integer measures. + + ``agg.mutate(share=t.total / t.all(t.total))`` routes ``t.all`` through + ``attach_windowed_totals``, whose ``__bsl_totals__`` column stayed int64 + — so on engines that integer-divide (xorq's DataFusion) every share came + back 0.0 while the identical calc-measure spelling was correct. The + totals column is now cast to float64 at creation, same policy as + ``MeasureScope.all``. + """ + df = ( + model.group_by("carrier") + .aggregate("total") + .mutate(share=lambda t: t.total / t.all(t.total)) + .execute() + ) + got = {k: pytest.approx(float(v)) for k, v in zip(df["carrier"], df["share"], strict=True)} + assert got == SUM_SHARE + + # Count measures ride the same lift. + counted = ( + model.with_measures(n=lambda t: t.count()) + .group_by("carrier") + .aggregate("n") + .mutate(share=lambda t: t.n / t.all(t.n)) + .execute() + ) + got = { + k: pytest.approx(float(v)) + for k, v in zip(counted["carrier"], counted["share"], strict=True) + } + assert got == {"A": 3 / 4, "B": 1 / 4} + + def test_percent_of_total_is_order_independent(): """Declaration order must not change the answer.""" data = ibis.memtable({"carrier": ["AA", "UA", "DL", "WN", "B6"] * 10}) @@ -140,24 +174,13 @@ def test_chain_mutate_after_order_by_is_refused(model): .order_by("carrier") .mutate(share=lambda t: t.total / t.all(t.total)) ) - # The surviving spellings agree with each other. + # The surviving spellings agree with each other — including on the + # memtable → canonical-backend path, where the integer totals column + # used to integer-divide to 0.0 before attach_windowed_totals gained + # the float64 cast. assert _shares(model.with_measures(share=lambda t: t.total / t.all(t.total))) == SUM_SHARE - # The direct-on-the-aggregate spelling, pinned on duckdb: the memtable → - # canonical-backend path truncates this integer ratio to 0.0 (pre-existing - # xorq/DataFusion flavor defect, independent of the mutate desugaring — - # the compiled SQL carries the float cast and duckdb executes it). - con = ibis.duckdb.connect(":memory:") - tbl = con.create_table( - "flights_chain", - {"carrier": ["A", "A", "A", "B"], "distance": [10, 20, 30, 100]}, - ) - duck_model = ( - to_semantic_table(tbl, "flights") - .with_dimensions(carrier=lambda t: t.carrier) - .with_measures(total=lambda t: t.distance.sum()) - ) df = ( - duck_model.group_by("carrier") + model.group_by("carrier") .aggregate("total") .mutate(share=lambda t: t.total / t.all(t.total)) .execute()