Skip to content

GH-51145: [Python] Accept Arrow integer scalars in slice methods - #51160

Merged
AlenkaF merged 2 commits into
apache:mainfrom
YusefSyed:codex/gh-51145-arrow-integer-slice
Sep 25, 2026
Merged

AlenkaF merged 2 commits into
apache:mainfrom
YusefSyed:codex/gh-51145-arrow-integer-slice

Conversation

@YusefSyed

@YusefSyed YusefSyed commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Rationale for this change

Array.slice, ChunkedArray.slice, RecordBatch.slice, and Table.slice
accept Python and NumPy integer-like values but reject Arrow integer scalars,
even though those scalars implement Python's integer-index protocol. This makes
values produced by Arrow APIs unnecessarily unusable as slice offsets and
lengths.

What changes are included in this PR?

Normalize non-None offsets and lengths with operator.index() at the four
public Python wrapper boundaries before the existing validation and C++ calls.

Add regression coverage for all signed and unsigned Arrow integer scalar types
across the four wrappers, plus NumPy compatibility, invalid and null scalars,
negative values, offset clamping, and int64 overflow behavior.

Are these changes tested?

Yes. Current Arrow C++ and editable PyArrow were built from source in an
isolated Python 3.13 environment. The focused new tests and existing slice
controls passed: 48 passed.

Cython translation, Python test-file compilation, Flake8, and
git diff --check also passed.

Are there any user-facing changes?

Yes. The four Python .slice() methods now accept non-null Arrow integer
scalars for offsets and lengths. Existing Python/NumPy integer behavior and
error behavior for unsupported values are preserved.

AI assistance

AI assisted analysis, implementation, test drafting, build work, and review.
The account holder reviewed and approved the final four-file diff. The account
holder did not personally run the commands reported above.

@YusefSyed
YusefSyed marked this pull request as ready for review September 4, 2026 15:58

@AlenkaF AlenkaF left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR! I was thinking of suggesting the use of Py_ssize_t-typed Cython parameter but operator.index() does solve everything: accepts Python ints and Arrow Scalars if they are integer type. cc @raulcd what do you think?

In any case, we need to update docstrings for offset and length as they do not need to be int but can also be Arrow Scalars.

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 10, 2026
Update offset and length documentation for all four slice methods. Arrow scalars must be non-null integers. Prepared and validated with OpenAI Codex assistance.

@raulcd raulcd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not an expert on this area but this seems reasonable to me.

@github-actions github-actions Bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Sep 14, 2026
@YusefSyed

Copy link
Copy Markdown
Contributor Author

Hi @AlenkaF and @raulcd, thanks for reviewing this. The offset and length docstrings have been updated across all four slice methods to cover non-null Arrow integer scalars. Following up after the September 14 approval: is there anything else needed from me before this can proceed?

Codex-assisted follow-up, posted with my approval.

@raulcd
raulcd requested a review from AlenkaF September 25, 2026 07:14
@raulcd

raulcd commented Sep 25, 2026

Copy link
Copy Markdown
Member

thanks for the ping @YusefSyed . @AlenkaF I think this is ok to merge, I'll wait in case you can take a quick look. I'll merge next week otherwise.

@AlenkaF AlenkaF left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the ping and the update!
I have no follow-up comments, will merge.

@AlenkaF
AlenkaF merged commit 7a0491f into apache:main Sep 25, 2026
38 checks passed
@AlenkaF AlenkaF removed the awaiting merge Awaiting merge label Sep 25, 2026
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.

3 participants