refactor: collapse date_bin month binning onto a single civil-calendar path - #1

Which issue does this PR close?

None directly — this targets your fix-25855-date-bin-month-stride branch, as a
suggested restructuring of apache#26153 (which closes apache#25855). Take it, take part of it,
or close it; I won't be offended either way.

Rationale for this change

No user-visible change. This is purely about the shape of the implementation.

Your fix is correct, and I verified the arithmetic against the chrono path across
the overlapping domain. My concern is structural: after the fix, date_bin has two
complete implementations of month-bin semantics.

Both of these encode the same five rules — month-index difference, compute_distance,
shift from the origin, clamp a missing day to the month end, and step back one stride
on overshoot:

Any future change to month semantics now has to land in both, in two numeric domains,
via two different libraries. test_date_bin_months_wide_matches_chrono_path is the
signal — a test whose job is to keep two implementations in sync is usually a sign
there should only be one.

The civil implementation you added is strictly more general than the chrono one, so
the chrono one can simply go.

What changes are included in this PR?

One commit on top of your branch.

Collapse the two month paths. bin_months_civil does the binning in i64, and both
widths become thin adapters over it — the i128 path exists only to widen at the
nanosecond boundary. bin_months, shift_months and to_utc_date_time drop out,
along with four of six chrono imports.

Move the calendar arithmetic to datetime::civil. civil_from_days and
days_from_civil were private to date_trunc and your PR opened them to pub(super)
so date_bin could reach them. That couples two sibling UDF modules and quietly makes
date_trunc a utility module for calendar math. They now live in their own module
alongside CivilTimestamp, which owns the nanosecond conversions, month_index and
shift_months. Both date_trunc and date_bin are plain consumers again.

days_in_month uses a lookup table instead of two days_from_civil calls. That
was ~12 integer divisions per row to get a month length, and it was the single most
expensive line in the new code — worth ~36% of the month path on its own.

Drop Option from helpers that cannot fail. month_index and civil_to_nanos
can't return None for any reachable input (source is bounded by i64::MAX * 10^9,
and the stride by i32 via IntervalMonthDayNano). Removing it leaves exactly one
real failure — whether the bin fits the output type — at the boundary where the caller
already checks it.

Document the Ord invariant. The derived ordering on CivilTimestamp is
load-bearing for bin > source, and depends on field declaration order plus two
normalizations. Both are now stated on the type, and every constructor lives in the
module that maintains them.

benches/datetime_expressions/date_bin.rs        |  36 ++++-
datafusion/functions/src/datetime/civil.rs      | 180 +++++++++++++++++
datafusion/functions/src/datetime/date_bin.rs   | 162 ++++---------
datafusion/functions/src/datetime/date_trunc.rs |  61 +------
datafusion/functions/src/datetime/mod.rs        |   1 +
5 files changed, 249 insertions(+), 191 deletions(-)

date_bin.rs goes 1920 -> 1820 lines (it was 1810 before your PR), and date_trunc.rs
1838 -> 1781.

What is the testing strategy for this PR?

Every test and slt expectation on your branch is unchanged — this PR touches no
test file and no .slt file, and your suite passes verbatim. That's the main evidence
that behavior is preserved.

• cargo test -p datafusion-functions — 394 passed, 0 failed
• date_bin_errors.slt, timestamps.slt, dates.slt, date_trunc.slt — all pass
• cargo clippy --all-targets --all-features -- -D warnings — clean

One caveat you should weigh. Because both widths now share a core,
test_date_bin_months_wide_matches_chrono_path still passes but no longer does what
its name says — it compares the two adapters, not civil-vs-chrono. If you take this,
that test is worth reworking to compute its expectation with chrono inside the test, so
chrono stays a test oracle pinning the semantics without being production machinery.
I left that out deliberately rather than quietly rewrite your test.

Benchmark. benches/datetime_expressions/date_bin.rs only covered day-time strides,
so the month path had no coverage; I added date_bin_months_1000. Measured with it,
same machine, same session, this branch vs your branch:

This is performance-neutral, not a speedup. The days_in_month lookup pays back
the cost of leaving chrono and no more. I'm claiming a structural improvement at no
runtime cost, not a faster date_bin.

Are there any user-facing changes?

No. No behavior change and no public API change — everything touched is either private
to date_bin.rs or crate-internal, and the new civil module is pub(crate). No
api change label needed.

🤖 Generated with Claude Code