Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

h-vetinari
Copy link
Contributor

@h-vetinari h-vetinari commented Nov 2, 2024

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?

Copy link

github-actions bot commented Nov 2, 2024

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

@h-vetinari
Copy link
Contributor Author

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
Copy link

github-actions bot commented Nov 5, 2024

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

@kou
Copy link
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
Contributor Author

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
Contributor Author

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

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
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
Copy link

github-actions bot commented Nov 7, 2024

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

@h-vetinari
Copy link
Contributor Author

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
Contributor Author

Gentle ping @kou

@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
@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
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
Member

kou commented Nov 14, 2024

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

@kou
Copy link
Member

kou commented Nov 14, 2024

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

@h-vetinari
Copy link
Contributor Author

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

Done

Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants