Skip to content

GH-50399: [C++][Gandiva] Use timegm in DaysSince helper in Gandiva date_time_test#50400

Merged
amoeba merged 1 commit into
apache:mainfrom
amoeba:GH-50399--c++-gandiva--time-zone-utils
Jul 8, 2026
Merged

GH-50399: [C++][Gandiva] Use timegm in DaysSince helper in Gandiva date_time_test#50400
amoeba merged 1 commit into
apache:mainfrom
amoeba:GH-50399--c++-gandiva--time-zone-utils

Conversation

@amoeba

@amoeba amoeba commented Jul 6, 2026

Copy link
Copy Markdown
Member

Rationale for this change

DateTimeTestProjector.TestDateTime fails for me locally and it looks like the issue is that it's comparing datetimes calculated with two different methods:

  1. Epoch(): Uses timegm as of GH-49952: [C++][Gandiva] Use timegm in date_time_test utilities #49953
  2. DaysSince() was still using mktime

What changes are included in this PR?

Updated DaysSince() helper to use timegm to match other helpers so comparisons can be made without local time one handling issues.

Are these changes tested?

Yes. The test that fails locally now passes locally with this.

Are there any user-facing changes?

No.

Copilot AI review requested due to automatic review settings July 6, 2026 23:54
@github-actions github-actions Bot added the awaiting review Awaiting review label Jul 6, 2026
@amoeba

amoeba commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

@Reranko05 @lriggs would either of you want to look at this PR?

@amoeba amoeba changed the title [C++][Gandiva] Use timegm in DaysSince helper in Gandiva date_time_test GH-50399: [C++][Gandiva] Use timegm in DaysSince helper in Gandiva date_time_test Jul 6, 2026
@apache apache deleted a comment from github-actions Bot Jul 6, 2026

Copilot AI 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.

Pull request overview

This PR fixes inconsistent timezone handling in Gandiva’s datetime test utilities by switching the DaysSince() helper from mktime() (local time) to timegm() (UTC), aligning it with Epoch() and other helpers so date/time comparisons are stable across platforms and DST settings.

Changes:

  • Updated DaysSince() to use timegm() instead of mktime().
  • Updated the fatal log message to match the new helper (timegm() failed).

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #50399 has been automatically assigned in GitHub to PR creator.

@Reranko05 Reranko05 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.

LGTM. Thanks!

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 7, 2026
@amoeba amoeba requested review from pitrou and rok July 7, 2026 19:58
@amoeba

amoeba commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

Thanks for looking @Reranko05 and @lriggs.

Let's get at least one committer/PMC review and then I'll merge.

@adamreeve adamreeve 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.

Looks good to me thanks @amoeba. I also saw this test failure when testing the 25.0.0 release, and can confirm that with this change the DateTimeTestProjector tests all pass for me.

@amoeba amoeba merged commit 6096d41 into apache:main Jul 8, 2026
59 checks passed
@amoeba amoeba removed the awaiting committer review Awaiting committer review label Jul 8, 2026
@amoeba

amoeba commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Thanks @adamreeve!

@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 6096d41.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 6096d41.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants