Skip to content

allow converting from DateTime2 to Datetimen in 'tds73' - #298

Closed
Geo-W wants to merge 2 commits into
tiberius-rs:mainfrom
Geo-W:datatime
Closed

Geo-W wants to merge 2 commits into
tiberius-rs:mainfrom
Geo-W:datatime

Conversation

@Geo-W

@Geo-W Geo-W commented Jun 2, 2023

Copy link
Copy Markdown

This pr tries to address the following problem:
When inserting NaiveDateTime into ms datetime columns with bulk_insert and tds73 is turned on by default, it would raise
Err` value: BulkInput("invalid data type, expecting Some(VarLenSized(VarLenContext { type: Datetimen, len: 8, collation: None })) but found DateTime2(Some(DateTime2 { date: Date(693604), time: Time { increments: 330110000000, scale: 7 } }))

cannot just disable tds73 feature as date/time might in the same table with datetime simultaneously and into_sql/to_sql is impl for date/time in without tds73 only.

normal inserting with execute does not get this issue as it calls DateTime2 to None when converting.
https://github.com/prisma/tiberius/blob/98943b21b5f7a5135d3ae4bd51acbf268bb99189/src/tds/codec/column_data.rs#L615-L619

Don't know whether it is a good solution. Or maybe any other impl can solve this issue is appreciated.
Thank you so much~

@NTmatter

Copy link
Copy Markdown
Contributor

I'm seeing a similar issue, but with a regular DATETIME column. If I were to make a feature request, I'd ask that the legacy DateTime conversion functionality be made available for manual use.

As a workaround, I've copied/modified the legacy PrimitiveDateTime handling from tds/time.rs (gated behind #[cfg(not(feature = "tds73"))]), spitting out a ColumnData::DateTime variant that can be fed into bulk insert's TokenRow.

It's a little extra work, but it prevents a mandatory conversion to DateTime2:

pub fn naive_dt_to_datetime1<'a>(dt: NaiveDateTime) -> ColumnData<'a> {
    fn to_days(date: NaiveDate, start_year: i32) -> i64 {
        (date - NaiveDate::from_ymd_opt(start_year, 1, 1).unwrap()).num_days()
    }

    fn to_sec_fragments(from: NaiveTime) -> i64 {
        let nanos: i64 = (from - NaiveTime::from_hms_opt(0, 0, 0).unwrap())
            .num_nanoseconds()
            .unwrap();

        nanos * 300 / (1e9 as i64)
    }

    let date = dt.date();
    let time = dt.time();

    let days = to_days(date, 1900) as i32;
    let seconds_fragments = to_sec_fragments(time);

    let dt = tiberius::time::DateTime::new(days, seconds_fragments as u32);
    ColumnData::DateTime(Some(dt))
}

For some example usage:

/// Convert my domain object into a row usable by bulk insert
impl<'a> IntoRow<'a> for EventRow {
    fn into_row(self) -> TokenRow<'a> {
        let mut row = TokenRow::new();

        row.push(naive_dt_to_datetime1(self.timestamp.naive_utc()));
        row.push(self.event_id.into_sql());

        row
    }
}

/// Perform bulk insert
async fn do_inserts(db: &Client, events: &Vec<EventRow>) -> anyhow::Result<()>{
    let mut bulk = db.bulk_insert("TABLE_NAME").await?;
    for evt in events {
        let row = evt.into_row();
        bulk.send(row).await?;
    }
    _ = bulk.finalize().await?;
   Ok(())
}

joelparkerhenderson added a commit to mssql-rust/mssql-rust that referenced this pull request Aug 29, 2026
Mirrors tiberius-rs/tiberius#298 (allow converting from DateTime2 to
Datetimen in 'tds73'). With tds73 enabled, bulk_insert into a `datetime`
column previously failed with a BulkInput "invalid data type" error for
any value that reached ColumnData as DateTime2 (which chrono/time's
NaiveDateTime/PrimitiveDateTime conversions always produce) -- unlike
Client::execute, which already downgrades DateTime2 to the legacy
DateTime type before encoding (see the sibling arm a few lines below).
Disabling tds73 isn't a workaround either, since `date`/`time` columns
in the same table need it.

Reimplemented rather than cherry-picked: the original PR's patch added
an unconditional `use chrono::{Duration, NaiveDate}` at the top of
column_data.rs (a file compiled regardless of the `chrono` feature),
which would have broken the default build (tds73 on, chrono off is
exactly cargo's default feature set). This version needs neither chrono
nor time -- date-days and seconds-fragment arithmetic reuses this
crate's own tds::time::DateTime type directly, so the new encode arm is
gated only by `tds73`, matching its sibling arms.

Also generalized the seconds-fragments conversion to use DateTime2's
actual `time().scale()` instead of hardcoding scale 7 (true for every
chrono/time-crate conversion today, but not guaranteed for a value
built directly via the public DateTime2::new/Time::new API), and fixed
the day-range check: the original rejected anything before 1900-01-01,
but `datetime`'s documented minimum -- and the actual boundary that
matters for a `datetime` column -- is 1753-01-01.

Verified: cargo check across all 6 CI feature combinations (including
plain `cargo check`, i.e. tds73 on / chrono off, which is the case the
original patch would have failed on), cargo clippy --all-targets, cargo
fmt --check, and cargo test --lib (146 passing, including two new tests
covering the DateTime2->Datetimen conversion and the 1753 boundary).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0156Di1tRRLsJK8ctU1AmAJr
MattJackson referenced this pull request in MattJackson/tiberius-ng Aug 29, 2026
joelparkerhenderson added a commit to mssql-rust/mssql-rust that referenced this pull request Aug 30, 2026
Mirrors tiberius-rs/tiberius#298 (allow converting from DateTime2 to
Datetimen in 'tds73'). With tds73 enabled, bulk_insert into a `datetime`
column previously failed with a BulkInput "invalid data type" error for
any value that reached ColumnData as DateTime2 (which chrono/time's
NaiveDateTime/PrimitiveDateTime conversions always produce) -- unlike
Client::execute, which already downgrades DateTime2 to the legacy
DateTime type before encoding (see the sibling arm a few lines below).
Disabling tds73 isn't a workaround either, since `date`/`time` columns
in the same table need it.

Reimplemented rather than cherry-picked: the original PR's patch added
an unconditional `use chrono::{Duration, NaiveDate}` at the top of
column_data.rs (a file compiled regardless of the `chrono` feature),
which would have broken the default build (tds73 on, chrono off is
exactly cargo's default feature set). This version needs neither chrono
nor time -- date-days and seconds-fragment arithmetic reuses this
crate's own tds::time::DateTime type directly, so the new encode arm is
gated only by `tds73`, matching its sibling arms.

Also generalized the seconds-fragments conversion to use DateTime2's
actual `time().scale()` instead of hardcoding scale 7 (true for every
chrono/time-crate conversion today, but not guaranteed for a value
built directly via the public DateTime2::new/Time::new API), and fixed
the day-range check: the original rejected anything before 1900-01-01,
but `datetime`'s documented minimum -- and the actual boundary that
matters for a `datetime` column -- is 1753-01-01.

Verified: cargo check across all 6 CI feature combinations (including
plain `cargo check`, i.e. tds73 on / chrono off, which is the case the
original patch would have failed on), cargo clippy --all-targets, cargo
fmt --check, and cargo test --lib (146 passing, including two new tests
covering the DateTime2->Datetimen conversion and the 1753 boundary).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0156Di1tRRLsJK8ctU1AmAJr
MattJackson added a commit that referenced this pull request Sep 3, 2026
MattJackson added a commit that referenced this pull request Sep 4, 2026
MattJackson added a commit that referenced this pull request Sep 6, 2026
MattJackson added a commit that referenced this pull request Sep 16, 2026
MattJackson added a commit that referenced this pull request Sep 24, 2026
@MattJackson

Copy link
Copy Markdown
Contributor

Included in #442 — shipped in tiberius v0.13.0. Thanks for this! Closing.

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.

3 participants