Skip to content

Fix daylight savings bug#285

Closed
composerinteralia wants to merge 1 commit intomainfrom
daylight-savings-bug
Closed

Fix daylight savings bug#285
composerinteralia wants to merge 1 commit intomainfrom
daylight-savings-bug

Conversation

@composerinteralia
Copy link
Copy Markdown
Collaborator

SELECT date_time_test FROM trilogy_test
SQL

assert_equal_timestamp time, results[0][0]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Interesting. So on an older version, this test would return:

2024-11-03 01:21:54 -0500

But now on main, it returns:

2024-11-03 01:21:54 -0400

So we have a weird off by one error, is that what you mean by "daylight saving" bug? Is that issue only during DLS period? That would suggest rb_time_timespec_new( doesn't handle this correctly?

If we're in need of a quick fix, I suppose we can fallback to creating Time the old way when QUERY_FLAGS_LOCAL_TIMEZONE is set. I suspect most users don't have that flag set.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Proposed fix here: #286

byroot pushed a commit to byroot/trilogy that referenced this pull request Apr 1, 2026
Fix: trilogy-libraries#284
Fix: trilogy-libraries#285

`rb_time_timespec_new` "local time" semantic isn't the same as the
Ruby level `Time.local` API.

`Time.local` will create a time with the TZ offset relative to that
time e.g. a June date will have summer offset regardless of the actual
current time. So that `Time.local(2024, 6, 3, 1, 21, 54)` will always
return the same thing no matter what.

Whereas `rb_time_timespec_new` will use the current offset for that
timezone, hence calling the C equivalent of `Time.local(2024, 6, 3, 1, 21, 54)`
will return different things over time.

Co-Authored-By: Daniel Colson <danieljamescolson@gmail.com>
@byroot byroot closed this in 8b3e151 Apr 1, 2026
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