GH-41605: [C++][Gandiva] Stabilize TestFromUtcTimestamp around DST boundary#49887
GH-41605: [C++][Gandiva] Stabilize TestFromUtcTimestamp around DST boundary#49887Reranko05 wants to merge 1 commit into
Conversation
|
|
github-actions
Bot
added
Component: C++
Component: Gandiva
awaiting review
|
I think this is ok, but I would consider adding a comment in the test code about the fragility around DST times. An alternative my AI assistant proposed is using timegm instead of mktime and then you can also remove the Hack in Epoch(). The only trick is that windows needs a timegm definition, like this: #ifdef _WIN32 / #define timegm _mkgmtime Apparently some implementations of mktime account for DST and others dont. That approach might be worth trying out locally to see if it fixes the failing test. #49902 |
Reranko05
force-pushed
the
41605-datetimetest
branch
from
b2219b7 to
b027fcc
Compare
|
@lriggs Added a comment in the test to highlight the DST-related fragility. I will try |
github-actions
Bot
added
awaiting committer review
### 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 Issue: #49952 Authored-by: Aaditya Srinivasan <aadityasri03@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
|
Can we close this in favor of #49953? |
|
OK. Let's close this. |
3 participants
Rationale for this change
The existing test includes a timestamp that falls on a DST spring-forward boundary, which can make the expected result environment-dependent. This change avoids that boundary to make the test deterministic across environments.
What changes are included in this PR?
Adjusted the DST-sensitive test input in
TestFromUtcTimestampUpdated the corresponding expected output
Are these changes tested?
Are there any user-facing changes?
No