Skip to content

Fix integer t.all() totals truncating to 0 on DataFusion - #307

Open
hussainsultan wants to merge 1 commit into
fix/refuse-raw-executefrom
fix/int-totals-truncation
Open

Fix integer t.all() totals truncating to 0 on DataFusion#307
hussainsultan wants to merge 1 commit into
fix/refuse-raw-executefrom
fix/int-totals-truncation

Conversation

@hussainsultan

Copy link
Copy Markdown
Collaborator

Stacked on #306 (fix/refuse-raw-execute), which surfaced this defect while pinning the surviving agg.mutate spelling.

Problem

model.group_by("carrier").aggregate("total") \
     .mutate(share=lambda t: t.total / t.all(t.total))

returned 0.0 for every share on the memtable → canonical-backend (xorq/DataFusion) path when the measure is integer-typed — silent-wrong, while the identical calc-measure spelling (with_measures(share=...)) was correct and the duckdb-compiled SQL carried a float cast.

Cause

attach_windowed_totals (calc_compiler.py) builds the lifted __bsl_totals__<name> column as agg_expr.over(window()) with the measure's original dtype. For an integer SUM/COUNT measure that column stays int64, and xorq's DataFusion executes ibis truediv on two int64 operands as integer division (60 / 160 → 0), even though ibis types the result float64:

t = xo.memtable({"a": [60, 100], "b": [160, 160]})
t.mutate(r=t.a / t.b).execute()   # r == 0.0, 0.0

The calc-measure path already guards against exactly this via _float_total in MeasureScope.all ("with two integer operands some engines do integer division, and 30/160 came back as 0"); the inline lift missed the same policy.

Fix

Apply the established integer→float64 cast where the totals columns are created:

  • attach_windowed_totals casts the windowed total (_float_total, integer dtypes only — non-numeric totals untouched),
  • attach_calc_totals casts calc-of-calc totals the same way, so an int-valued calc can't reintroduce the truncation downstream.

Casting the denominator makes the division int/float64, which DataFusion executes correctly (verified: 0.375 / 0.625).

Tests

  • New test_integer_ratio_via_aggregate_mutate_is_a_ratio covers integer sum and count measures through the inline lift on the memtable fixture.
  • The duckdb pin added in Refuse raw output from definition-side semantic expressions #306 is removed — the inline spelling now asserts SUM_SHARE on the shared memtable → DataFusion path directly.
  • Full suite: 1335 passed + 13 integration (Malloy) passed.

🤖 Generated with Claude Code

agg.mutate(share=lambda t: t.total / t.all(t.total)) routes t.all through
attach_windowed_totals, whose __bsl_totals__<name> 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 <noreply@anthropic.com>
@hussainsultan
hussainsultan force-pushed the fix/int-totals-truncation branch from a04c332 to 1de4f0c Compare August 24, 2026 06:16
@hussainsultan
hussainsultan marked this pull request as ready for review August 24, 2026 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant