Skip to content

GH-43808: [C++] skip -0117 in StrptimeZoneOffset for old glibc#44621

Merged
kou merged 3 commits intoapache:mainfrom
h-vetinari:glibc_tz
Nov 15, 2024
Merged

GH-43808: [C++] skip -0117 in StrptimeZoneOffset for old glibc#44621
kou merged 3 commits intoapache:mainfrom
h-vetinari:glibc_tz

Conversation

@h-vetinari
Copy link
Copy Markdown
Contributor

@h-vetinari h-vetinari commented Nov 2, 2024 •
edited by kou
Loading

Rationale for this change

Enable tests for libarrow in conda-forge: #35587

What changes are included in this PR?

old glibc does not actually support timezones like -0117 (used in StrptimeZoneOffset test). The exact lower bound for glibc is hard for me to determine; I know that it passes with 2.28 and that it fails with 2.17. Anything in between is an open question. I went with the conservative option here.

Are these changes tested?

Tested in conda-forge/arrow-cpp-feedstock#1058

Are there any user-facing changes?

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 2, 2024

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

@h-vetinari h-vetinari mentioned this pull request Nov 2, 2024
Closed
@h-vetinari
Copy link
Copy Markdown
Contributor Author

h-vetinari commented Nov 5, 2024

PTAL @kou :)

CC @pitrou

@kou kou changed the title GH-43808: [C++] set kStrptimeSupportsZone to false for old glibc GH-43808: [C++] Set kStrptimeSupportsZone to false for old glibc Nov 5, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 5, 2024

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

@kou
Copy link
Copy Markdown
Member

kou commented Nov 5, 2024

Hmm. It seems that we need to improve the added condition...

Java JNI / AMD64 manylinux2014 Java JNI: https://github.com/apache/arrow/actions/runs/11692780048/job/32562887155?pr=44621#step:7:3448

[ RUN      ] TimestampConversion.UserDefinedParsersWithZone
/arrow/cpp/src/arrow/csv/converter_test.cc:169: Failure
Failed
Expected 'converter->Convert(*parser, i)' to fail with Invalid, but got OK
/arrow/cpp/src/arrow/csv/converter_test.cc:169: Failure
Failed
Expected 'converter->Convert(*parser, i)' to fail with Invalid, but got OK
[  FAILED  ] TimestampConversion.UserDefinedParsersWithZone (0 ms)

C++ / AMD64 Conda C++ AVX2: https://github.com/apache/arrow/actions/runs/11692780055/job/32562895558?pr=44621#step:7:4505

[ RUN      ] TimestampConversion.UserDefinedParsersWithZone
/arrow/cpp/src/arrow/csv/converter_test.cc:169: Failure
Failed
Expected 'converter->Convert(*parser, i)' to fail with Invalid, but got OK
/arrow/cpp/src/arrow/csv/converter_test.cc:169: Failure
Failed
Expected 'converter->Convert(*parser, i)' to fail with Invalid, but got OK
[  FAILED  ] TimestampConversion.UserDefinedParsersWithZone (0 ms)

@h-vetinari
Copy link
Copy Markdown
Contributor Author

h-vetinari commented Nov 6, 2024

Hmm. It seems that we need to improve the added condition...

Or just the test expectations need to depend on kStrptimeSupportsZone?

@h-vetinari
Copy link
Copy Markdown
Contributor Author

h-vetinari commented Nov 6, 2024

FWIW, I don't mind how this is done. We could also add

#if defined(__GLIBC__) && defined(__GLIBC_MINOR__) && (__GLIBC__ == 2) && (__GLIBC_MINOR__ < 28)

in

arrow/cpp/src/arrow/util/value_parsing_test.cc

Lines 829 to 832 in 11c11a4

TEST(TimestampParser, StrptimeZoneOffset) {
if (!kStrptimeSupportsZone) {
GTEST_SKIP() << "strptime does not support %z on this platform";
}

directly, but the reason I went with this is that my understanding is that it's unlike that old glibc actually supports this feature, and we might as well reflect that...?

@kou
Copy link
Copy Markdown
Member

kou commented Nov 6, 2024

I think that the test failure shows that glibc used by these CI supports %z in strptime(). (Right?)
If so, we should set kStrptimeSupportsZone to true for the glibc.

@h-vetinari h-vetinari changed the title GH-43808: [C++] Set kStrptimeSupportsZone to false for old glibc GH-43808: [C++] skip -0117 in StrptimeZoneOffset for old glibc Nov 7, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 7, 2024

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

@h-vetinari
Copy link
Copy Markdown
Contributor Author

h-vetinari commented Nov 7, 2024

If so, we should set kStrptimeSupportsZone to true for the glibc.

You're right, that existing skip was a red herring. I think the only issue is with the -0117 timezone.

@h-vetinari h-vetinari force-pushed the glibc_tz branch 2 times, most recently from a3f3105 to 5ce5c5d Compare November 7, 2024 01:27
@h-vetinari
Copy link
Copy Markdown
Contributor Author

h-vetinari commented Nov 11, 2024

Gentle ping @kou

kou
kou reviewed Nov 11, 2024
View reviewed changes
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Nov 11, 2024
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 11, 2024
kou
kou reviewed Nov 14, 2024
View reviewed changes
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Nov 14, 2024
@github-actions github-actions bot removed the awaiting changes Awaiting changes label Nov 14, 2024
@github-actions github-actions bot added the awaiting change review Awaiting change review label Nov 14, 2024
@raulcd
Copy link
Copy Markdown
Member

raulcd commented Nov 14, 2024

The AMD64 Windows MinGW CLANG64 C++ job has just started failing but it does not seem related to the changes on the PR.

@kou
Copy link
Copy Markdown
Member

kou commented Nov 14, 2024

It's unrelated. I've opened a new issue for it: #44730

@kou
Copy link
Copy Markdown
Member

kou commented Nov 14, 2024

@h-vetinari Could you update the PR description before we merge this?

@h-vetinari
Copy link
Copy Markdown
Contributor Author

h-vetinari commented Nov 14, 2024

@h-vetinari Could you update the PR description before we merge this?

Done

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 14, 2024

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

kou
kou approved these changes Nov 15, 2024
View reviewed changes
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 3f25672 into apache:main Nov 15, 2024
@kou kou removed the awaiting change review Awaiting change review label Nov 15, 2024
@github-actions github-actions bot added the awaiting merge Awaiting merge label Nov 15, 2024
@kou kou mentioned this pull request Nov 15, 2024
Closed
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 15, 2024

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

@h-vetinari h-vetinari deleted the glibc_tz branch November 15, 2024 09:03
@conbench-apache-arrow
Copy link
Copy Markdown

conbench-apache-arrow bot commented Nov 15, 2024

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 49 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

Reviewers

@kou kou kou approved these changes

Assignees

No one assigned

Labels

awaiting merge Awaiting merge Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@h-vetinari @kou @raulcd