Skip to content

Howard Hinnant civil-date algorithms hand-written 7 times across 3 crates #6997

Open

Description

@andygrove

Describe the bug

Problem

Location: native/spark-expr/src/kernels/temporal.rs:402
Symbol: civil_from_days / days_from_civil

Grep finds days_from_civil and civil_from_days as separate private copies in: (1) native/spark-expr/src/kernels/temporal.rs:407 (civil_from_days) and :425 (days_from_civil); (2) native/spark-expr/src/conversion_funcs/string.rs:1300 (days_from_civil), plus a test copy at :3440; (3) native/core/src/parquet/datetime_rebase.rs:162 (days_from_civil); (4) native/spark-expr/src/iceberg_funcs/temporal.rs:132 (civil_from_days, i32 in, returns (year, month0)); (5) native/core/src/execution/operators/iceberg_partition_path.rs:278 (civil_from_days, i64 in, (i64,u32,u32) out). The copies have already drifted in how they do floor division: string.rs uses if y >= 0 { y / 400 } else { (y - 399) / 400 }, iceberg_partition_path.rs uses a manual shifted - 146_096 floor plus a cast to u64, and the others use div_euclid. Month indexing also differs ((m + 9) % 12 vs. if/else). Each copy is a separately maintained implementation of the same correctness-critical calendar math that Spark compatibility depends on.

Suggested fix:

Keep the div_euclid i64 versions from kernels/temporal.rs as pub fn days_from_civil(y, m, d) -> i64 and pub fn civil_from_days(days) -> (i64, i64, i64) in a crate that both spark-expr and core depend on (for example native/common, or a calendar module in spark-expr). Replace the other copies with thin adapters, such as (year as i32, month - 1) for Iceberg, and add one shared property test that checks the pair round-trips across the full i32 epoch-day range.

Code context

// Integer proleptic Gregorian calendar conversion, adapted from DataFusion 55.1.0's
// datetime/date_trunc.rs (Howard Hinnant's algorithms):
fn civil_from_days(days: i64) -> (i64, i64, i64) {
    let z = days + 719_468;
    let era = z.div_euclid(146_097);
    ...
}

fn days_from_civil(year: i64, month: i64, day: i64) -> i64 {
    let year = year - i64::from(month <= 2);
    let era = year.div_euclid(400);
    ...
    era * 146_097 + day_of_era - 719_468
}
// other copies: conversion_funcs/string.rs:1300, core/parquet/datetime_rebase.rs:162,
// iceberg_funcs/temporal.rs:132, core/.../iceberg_partition_path.rs:278

Steps to reproduce

No response

Expected behavior

No response

Additional context

No response

Willingness to contribute

None

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions