Skip to content

GH-35448: [C++] Fix detection of %z in strptime format #35449

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.

Sign up for GitHub

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

Jump to bottom
Merged
pitrou merged 3 commits into from
May 16, 2023
Merged

GH-35448: [C++] Fix detection of %z in strptime format #35449

pitrou merged 3 commits into from
May 16, 2023

Conversation

@jorisvandenbossche
Copy link
Member

@jorisvandenbossche jorisvandenbossche commented May 5, 2023 •
edited by github-actions bot
Loading

Rationale for this change

See gh-35448 for the failing example. The current code in GetZone was assuming there was always some character between the %z and the preceding % code (like a whitespace, or - or /). That is often not the case with %z (in time formats like 00:00+01, the + is part of %z, and so the format is %H:%M%z without character between %M and %z)

Are these changes tested?

Test is added

Are there any user-facing changes?

The result type will no now correctly have a tz=UTC parametrization

@github-actions
Copy link

github-actions bot commented May 5, 2023

@github-actions
Copy link

github-actions bot commented May 5, 2023

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

kou
kou approved these changes May 5, 2023
View reviewed changes
Copy link
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

cpp/src/arrow/compute/kernels/scalar_string_test.cc Outdated
Comment on lines 1900 to 1902
StrptimeOptions options("%m/%d/%Y %z", TimeUnit::MICRO, /*error_is_null=*/true);
this->CheckUnary("strptime", input1, timestamp(TimeUnit::MICRO, "UTC"), output1,
&options);
Copy link
Member

@kou kou May 5, 2023

Choose a reason for hiding this comment

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

Could you append 1 to options (options1) for consistency?

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels May 5, 2023
pitrou
pitrou approved these changes May 16, 2023
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1, thanks @jorisvandenbossche

@pitrou pitrou merged commit 0980dbe into apache:main May 16, 2023
@jorisvandenbossche jorisvandenbossche deleted the gh-35448 branch May 16, 2023 15:26
@jorisvandenbossche jorisvandenbossche mentioned this pull request May 17, 2023
Closed
@ursabot
Copy link

ursabot commented May 19, 2023

Benchmark runs are scheduled for baseline = 7fd6461 and contender = 0980dbe. 0980dbe is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.58% ⬆️0.15%] test-mac-arm
[Finished ⬇️2.54% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.36% ⬆️0.06%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 0980dbe3 ec2-t3-xlarge-us-east-2
[Finished] 0980dbe3 test-mac-arm
[Finished] 0980dbe3 ursa-i9-9960x
[Finished] 0980dbe3 ursa-thinkcentre-m75q
[Finished] 7fd64613 ec2-t3-xlarge-us-east-2
[Finished] 7fd64613 test-mac-arm
[Finished] 7fd64613 ursa-i9-9960x
[Finished] 7fd64613 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented May 19, 2023

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

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

@westonpace westonpace westonpace approved these changes

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

[C++] Detection of %z in strptime format fails in certain cases, not returning timestamp with tz=UTC

5 participants

@jorisvandenbossche @ursabot @kou @westonpace @pitrou