Skip to content

GH-50530: [Python] Assert assume_timezone results at input precision - #50368

Open
anxkhn wants to merge 2 commits into
apache:mainfrom
anxkhn:test/assume-timezone-missing-assert
Open

GH-50530: [Python] Assert assume_timezone results at input precision#50368
anxkhn wants to merge 2 commits into
apache:mainfrom
anxkhn:test/assume-timezone-missing-assert

Conversation

@anxkhn

@anxkhn anxkhn commented Jul 4, 2026

Copy link
Copy Markdown

Rationale for this change

Four .equals() calls in test_assume_timezone discard their boolean results, so the nonexistent-time and ambiguous-time branches are exercised but not verified.

Closes #50530.

What changes are included in this PR?

Assert all four comparisons. The expected arrays are constructed with the kernel result type so the test verifies values and preserves the input timestamp precision across pandas versions.

Are these changes tested?

A targeted execution of all four DST cases passes with pandas 3.0.5 and PyArrow 25.0.0, including exact type checks. git diff --check passes. The source-tree pytest requires a local Arrow C++/PyArrow build, which is not available in this checkout.

Are there any user-facing changes?

No.

This change was made with help from an AI coding assistant. I reviewed the diff and verified the expected/result pairings.

Copilot AI review requested due to automatic review settings July 4, 2026 23:19
@anxkhn
anxkhn requested review from AlenkaF, raulcd and rok as code owners July 4, 2026 23:19
@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes test_assume_timezone in python/pyarrow/tests/test_compute.py so that four previously non-asserted .equals() comparisons actually validate the assume_timezone results for DST-nonexistent and DST-ambiguous inputs.

Changes:

  • Add missing assert keywords for two nonexistent-time (shift_forward / shift_backward) result comparisons.
  • Add missing assert keywords for two ambiguous-time (earliest / latest) result comparisons.

@kou
kou marked this pull request as draft July 5, 2026 02:52
@kou

kou commented Jul 5, 2026

Copy link
Copy Markdown
Member

Please read https://arrow.apache.org/docs/dev/developers/overview.html carefully and try again.

@anxkhn anxkhn changed the title GH-<id>: [Python] Assert assume_timezone nonexistent/ambiguous results in test_compute GH-50530: [Python] Assert assume_timezone nonexistent and ambiguous results Jul 17, 2026
@github-actions

Copy link
Copy Markdown

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

@anxkhn

anxkhn commented Jul 17, 2026

Copy link
Copy Markdown
Author

I reread the contributing overview and corrected the submission: opened #50530, updated the title and issue linkage, replaced the accidental drafting notes with a concise PR description, and retained a clear AI-assistance disclosure.

@anxkhn
anxkhn force-pushed the test/assume-timezone-missing-assert branch from 1a79177 to c055e01 Compare July 17, 2026 16:34
@kou
kou marked this pull request as ready for review July 17, 2026 21:19
Copilot AI review requested due to automatic review settings July 17, 2026 21:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@kou

kou commented Jul 17, 2026

Copy link
Copy Markdown
Member

@AlenkaF Could you take a look at this? #14631 may be related.

@kou

kou commented Jul 18, 2026

Copy link
Copy Markdown
Member

Could you check the test failure?

https://github.com/apache/arrow/actions/runs/29596691304/job/87995650742?pr=50368#step:6:4568

>       assert expected.equals(result)
E       assert False
E        +  where False = equals(<pyarrow.lib.TimestampArray object at 0x5d9fbac2c40>\n[\n  2015-03-29 01:00:00.000000000Z,\n  2015-03-29 01:30:00.000000000Z\n])
E        +    where equals = <pyarrow.lib.TimestampArray object at 0x5d9fbac7ef0>\n[\n  2015-03-29 01:00:00.000000Z,\n  2015-03-29 01:30:00.000000Z\n].equals

FYI: You can check CI results on your fork too.

@rok

rok commented Jul 20, 2026

Copy link
Copy Markdown
Member

@AlenkaF Could you take a look at this? #14631 may be related.

Yes. Another potential source of this issue is if pandas and pyarrow were using different tzdb data, see #48743.

Copilot AI review requested due to automatic review settings July 27, 2026 17:51
@anxkhn
anxkhn force-pushed the test/assume-timezone-missing-assert branch from c055e01 to 3f68ea4 Compare July 27, 2026 17:51
@anxkhn anxkhn changed the title GH-50530: [Python] Assert assume_timezone nonexistent and ambiguous results GH-50530: [Python] Assert assume_timezone results at input precision Jul 27, 2026
@anxkhn

anxkhn commented Jul 27, 2026

Copy link
Copy Markdown
Author

Pushed a revision addressing the latest review/CI feedback.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

python/pyarrow/tests/test_compute.py:2809

  • Same as above: avoid type=result.type for expected; use an explicit timestamp(ns, tz) type so the assertion catches type/unit regressions.
    expected = pa.array(nonexistent.tz_localize(
        timezone, nonexistent="shift_backward"), type=result.type)

python/pyarrow/tests/test_compute.py:2826

  • Using type=result.type here makes the expected value dependent on the actual output type and can hide regressions. Use an explicit expected pa.timestamp("ns", timezone) instead.
    expected = pa.array(ambiguous.tz_localize(
        timezone, ambiguous=[True, True, True]), type=result.type)

python/pyarrow/tests/test_compute.py:2832

  • Same issue: avoid deriving expected type from result.type; set the expected Arrow type explicitly so the assertion validates the kernel’s output type/unit.
    expected = pa.array(ambiguous.tz_localize(
        timezone, ambiguous=[False, False, False]), type=result.type)

Comment thread python/pyarrow/tests/test_compute.py Outdated
@anxkhn

anxkhn commented Jul 27, 2026

Copy link
Copy Markdown
Author

fixed the ci failure by constructing each expected array with the kernel result type, so pandas timestamp inference cannot change the comparison precision. the four dst cases pass in a targeted check with pandas 3.0.5, and the branch is rebased with a signed and signed-off commit. @kou could you please re-review?

@rok

rok commented Jul 27, 2026

Copy link
Copy Markdown
Member

@anxkhn see my comment #50368 (comment). Can you post a more verbose output from pytest so we see where the difference actually is?

@anxkhn
anxkhn force-pushed the test/assume-timezone-missing-assert branch from 3f68ea4 to 9452bf7 Compare July 27, 2026 18:11
Copilot AI review requested due to automatic review settings July 27, 2026 18:11
@anxkhn
anxkhn force-pushed the test/assume-timezone-missing-assert branch from 9452bf7 to 88fd4b6 Compare July 27, 2026 18:12
@anxkhn

anxkhn commented Jul 27, 2026

Copy link
Copy Markdown
Author

updated the commit message to match the final implementation: expected types now come from the input timestamp unit and requested timezone. @kou could you please re-review the latest revision?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 27, 2026 18:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@rok

rok commented Jul 27, 2026

Copy link
Copy Markdown
Member

@anxkhn are you actually reading this thread or are you using an agent?

anxkhn added 2 commits July 28, 2026 00:09
…ision

Assert the existing nonexistent and ambiguous time comparisons so regressions cannot pass unnoticed. Construct expected arrays from the input timestamp unit and requested timezone to preserve precision across pandas versions without masking output-type regressions.

Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 27, 2026 18:39
@anxkhn
anxkhn force-pushed the test/assume-timezone-missing-assert branch from 88fd4b6 to a104cd4 Compare July 27, 2026 18:39
@anxkhn

anxkhn commented Jul 27, 2026

Copy link
Copy Markdown
Author

Pushed a revision addressing the latest review/CI feedback.

@anxkhn

anxkhn commented Jul 27, 2026

Copy link
Copy Markdown
Author

the verbose pandas-latest failure shows this is a resolution mismatch, not a timezone-data mismatch: pandas produced 2015-03-29 00:59:59.999999000z, while assume_timezone produced 2015-03-29 00:59:59.999999999z; both used europe/brussels and agreed on the offset. pandas 3.0.5 inferred the fixture as datetime64[us], but the arrow input is explicitly timestamp[ns]. i pushed a one-line fix that applies .as_unit("ns") to the pandas fixture before localization. pytest -vv test_compute.py::test_assume_timezone now reports 1 passed with pandas 3.0.5 and pandas 2.0.3. @rok could you please re-review?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@rok

rok commented Jul 27, 2026

Copy link
Copy Markdown
Member

@anxkhn rather than hardcoding to "ns" make the test parametric to units and test all (s, ms, us, ns). Make the asserts with equalities oneliners again.

@rok

rok commented Jul 31, 2026

Copy link
Copy Markdown
Member

Ping @anxkhn

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.

[Python] test_assume_timezone does not assert nonexistent and ambiguous results

4 participants