Skip to content

A MySQL TIME past a day or below zero reads as itself, so TIMEDIFF of 30 hours is 30:15:00, not null - #1411

Open
rudycelekli wants to merge 1 commit into
deeplethe:devfrom
rudycelekli:mysql-time-duration
Open

rudycelekli wants to merge 1 commit into
deeplethe:devfrom
rudycelekli:mysql-time-duration

Conversation

@rudycelekli

Copy link
Copy Markdown
Contributor

Why

A MySQL TIME is a signed duration from -838:59:59 to 838:59:59, not a time of day. TIMEDIFF, SEC_TO_TIME and TIME columns that hold elapsed hours all return it. The MySQL engine read it as chrono::NaiveTime, and that read has two problems.

  • 24 hours or more. sqlx refuses the conversion. The text fallback cannot decode TIME either, so the cell is null. TIMEDIFF('2024-01-02 06:15:00', '2024-01-01 00:00:00') came back as null instead of 30:15:00.
  • Negative values. The conversion drops the sign. A delivery that was 1h30 early, -01:30:00, came back as 01:30:00, so the model reads it as 1h30 late.

What changes

Cell::Time reads sqlx::mysql::types::MySqlTime, which covers the full range. It is written the way MySQL prints it: an optional -, zero-padded HH:MM:SS, and .ffffff only when there are microseconds. Ordinary times of day, such as 08:05:00, read the same as before.

The sign is taken from sign(). In sqlx 0.8.6, MySqlTime::is_negative() returns sign.is_positive(). Using it would invert every value, and the test catches that.

How it was checked

  • A new live test, a_time_past_a_day_or_below_zero_reads_as_itself_mysql, selects two TIMEDIFFs and CAST('08:05:00' AS TIME). It expects {"late":"30:15:00","early":"-01:30:00","start":"08:05:00"}.
  • UTOPIA_TEST_MYSQL_URL=mysql://root:pw@127.0.0.1:13306/sales cargo test -p utopia-server --bins query_engine::mysql, run against mariadb:11.4: 8 passed.
  • With the fix reverted, the test fails with left: ["{\"late\":null,\"early\":\"01:30:00\",\"start\":\"08:05:00\"}"].
  • cargo fmt --all --check and cargo clippy -p utopia-server --all-targets -- -D warnings pass.

Before review

  • Every commit is signed off (git commit -s)
  • cargo fmt --all --check, cargo clippy and cargo test pass for the touched crate

@WaylandYang WaylandYang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks right, and good catch on is_negative. CI has no MySQL, so the live test never runs there. Please add a unit test of time_text built with MySqlTime::new (covering >24h, negative and microseconds). Please also keep chrono's fractional style (.500, not .500000) so existing output doesn't change.

… 30 hours is 30:15:00, not null

Signed-off-by: Rudy Celekli <rudy@gradiahq.com>
@rudycelekli

Copy link
Copy Markdown
Contributor Author

Rebased on dev after #1410 and #1413. time_text now writes fractions the way chrono NaiveTime does (.500, and .000500 only when it is not whole milliseconds), so output for a TIME within a day does not change. A new unit test, a_time_is_written_as_the_duration_it_holds, builds MySqlTime::new values over 24 h, negative and fractional, so it runs in CI without MySQL.

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.

2 participants