Skip to content

GH-49952: [C++][Gandiva] Use timegm in date_time_test utilities#49953

Merged
kou merged 1 commit into
apache:mainfrom
Reranko05:gandiva-timegm-cleanup
Jun 5, 2026
Merged

GH-49952: [C++][Gandiva] Use timegm in date_time_test utilities#49953
kou merged 1 commit into
apache:mainfrom
Reranko05:gandiva-timegm-cleanup

Conversation

@Reranko05
Copy link
Copy Markdown
Contributor

@Reranko05 Reranko05 commented May 8, 2026

Rationale for this change

As discussed in PR #49887, the Gandiva datetime test utilities currently use mktime(), which interprets timestamps as local time and can behave differently across platforms and DST boundaries.

This PR switches the test utilities to timegm() so timestamps are interpreted consistently as UTC. This also removes the existing workaround logic in Epoch() that was previously needed for mktime() behavior on Windows/MSVC.

What changes are included in this PR?

  • replaced mktime() with timegm() in date_time_test.cc
  • added a small Windows compatibility mapping for _mkgmtime
  • removed the existing DST/localtime workaround logic in Epoch()

Are these changes tested?

  • Re-ran DateTimeTestProjector.TestFromUtcTimestamp repeatedly locally
  • Re-ran the full gandiva-projector-test suite successfully

Are there any user-facing changes?

No.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2026

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

@Reranko05
Copy link
Copy Markdown
Contributor Author

@lriggs I tried the timegm() approach suggested in #49887 locally and opened this follow-up PR based on it.

Copy link
Copy Markdown
Contributor

@lriggs lriggs left a comment

Choose a reason for hiding this comment

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

Thanks for the follow up.

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 13, 2026
@Reranko05
Copy link
Copy Markdown
Contributor Author

@kou This PR has CI passing and an approval from @lriggs. Would you have time for a committer review when convenient? Thanks!

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 updates Gandiva’s C++ datetime test utilities to interpret tm inputs as UTC (rather than local time) to avoid DST / timezone-dependent behavior in tests.

Changes:

  • Replaced mktime() with timegm() in UTC timestamp utilities used by tests.
  • Added a Windows compatibility mapping so timegm() resolves to _mkgmtime on _WIN32.
  • Simplified Epoch() by removing the prior MSVC/localtime workaround.
Comments suppressed due to low confidence (1)

cpp/src/gandiva/tests/date_time_test.cc:95

  • DaysSince() still uses mktime() (local time) even though Epoch()/MillisSince() now use UTC (timegm()). This keeps part of the date/time test utilities timezone/DST-dependent and contradicts the PR goal of making the tests deterministic. Switch DaysSince() to timegm() (with the existing Windows mapping) and update the error message accordingly.
  // time_t is an arithmetic type on both POSIX and Windows, we can simply
  // subtract to get a duration in seconds.
  return static_cast<int64_t>(ts - base_line) * 1000 + millis;
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@kou kou merged commit d0c59fa into apache:main Jun 5, 2026
54 checks passed
@kou kou removed the awaiting committer review Awaiting committer review label Jun 5, 2026
@github-actions github-actions Bot added the awaiting merge Awaiting merge label Jun 5, 2026
@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 d0c59fa.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.

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.

4 participants