GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty by koenvo · Pull Request #46057 · apache/arrow

koenvo · 2025-04-08T14:08:35Z

Rationale for this change

When filtering a RecordBatch using an expression that results in zero matching rows, an IndexError was raised due to an attempt to access the first batch of an empty result. This change ensures that such cases are handled gracefully by returning an empty RecordBatch with the correct schema.

What changes are included in this PR?

Fixed a bug in _filter_table where filtering all rows from a RecordBatch would raise an IndexError.

Added a helper function _empty_record_batch_from_schema to construct an empty RecordBatch with the correct schema.

Are these changes tested?

Yes, the behavior was verified manually. Let me know if you'd like a dedicated test case added to the test suite.

Are there any user-facing changes?

No changes to public APIs. The fix ensures more robust handling of edge cases internally.

GitHub Issue: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty #44366

github-actions · 2025-04-08T14:09:03Z

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:

Fokko · 2025-04-08T14:10:18Z

cc @wgtmac

github-actions · 2025-04-08T14:26:54Z

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

wgtmac · 2025-04-08T14:34:02Z

Thanks for the fix! It looks reasonable to me. However, I'm not familiar with the convention here. Perhaps @jorisvandenbossche @pitrou can help.

raulcd · 2025-04-08T14:56:18Z

Thanks for the PR!

AlenkaF

Thank you for contributing a fix for this.
I have one comment that will need a small change, otherwise the changes look good!

AlenkaF

Looks great to me now, thank you for all the updates!
Will wait for @raulcd to give another round of review and for the CI to finish before merging.

koenvo · 2025-04-09T10:08:14Z

Thank you all so much for the guidance!

raulcd

Thanks for the PR!
The CI failure is unrelated, I've opened to follow up:
#46077

github-actions · 2025-04-09T11:07:08Z

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

conbench-apache-arrow · 2025-04-09T19:57:22Z

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them.

…es error if result set is empty (apache#46057) ### Rationale for this change When filtering a RecordBatch using an expression that results in zero matching rows, an IndexError was raised due to an attempt to access the first batch of an empty result. This change ensures that such cases are handled gracefully by returning an empty RecordBatch with the correct schema. ### What changes are included in this PR? Fixed a bug in _filter_table where filtering all rows from a RecordBatch would raise an IndexError. Added a helper function _empty_record_batch_from_schema to construct an empty RecordBatch with the correct schema. ### Are these changes tested? Yes, the behavior was verified manually. Let me know if you'd like a dedicated test case added to the test suite. ### Are there any user-facing changes? No changes to public APIs. The fix ensures more robust handling of edge cases internally. - Github issue: apache#44366 * GitHub Issue: apache#44366 Authored-by: Koen Vossen <info@koenvossen.nl> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>

WillAyd · 2025-04-16T22:56:25Z

@github-actions crossbow submit -g python

github-actions · 2025-04-16T22:59:14Z

Revision: b5ef330

Submitted crossbow builds: ursacomputing/crossbow @ actions-85876e4ee6

Task	Status
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
test-conda-python-3.10
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest-numpy-latest
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-latest-numpy-1.26
test-conda-python-3.11-pandas-latest-numpy-latest
test-conda-python-3.11-pandas-nightly-numpy-nightly
test-conda-python-3.11-pandas-upstream_devel-numpy-nightly
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.12-cpython-debug
test-conda-python-3.13
test-conda-python-3.9
test-conda-python-3.9-pandas-1.1.3-numpy-1.19.5
test-conda-python-emscripten
test-cuda-python-ubuntu-22.04-cuda-11.7.1
test-debian-12-python-3-amd64
test-debian-12-python-3-i386
test-fedora-39-python-3
test-ubuntu-22.04-python-3
test-ubuntu-22.04-python-313-freethreading
test-ubuntu-24.04-python-3

WillAyd · 2025-04-16T23:11:03Z

I noticed this change caused a very minor regression with the example-python-minimal-build-ubuntu-venv and example-python-minimal-build-fedora-conda jobs. You can see the failing test here:

https://github.com/ursacomputing/crossbow/actions/runs/14504182883/job/40690374677#step:3:13332

From what I can tell, the added code here ends up referencing code from the pyarrow.acero module. However, it is possible to have a pyarrow configuration where compute is enabled but acero is not. The tests are failing in that case

AlenkaF · 2025-04-17T08:23:14Z

I may have mistakenly suggested moving the tests to test_compute.py, but they should actually remain in test_acero.py. I'll create an issue and a PR to fix this. Thanks @WillAyd for looking into it!

raulcd · 2025-04-17T08:48:01Z

or mark them as @pytest.mark.acero

AlenkaF · 2025-04-17T08:52:03Z

or mark them as @pytest.mark.acero

True! I already opened a PR: #46175. Can change to adding a mark if there will be a preference for it.

…ndexError RecordBatch.filter(Expression) raises IndexError on PyArrow 17-20 when the result has zero rows (fixed upstream in apache/arrow#46057). Replace the try/except approach with the Table-based workaround that avoids the bug, consistent with the pattern already used for the non-positional- deletes path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Fix IndexError when filtering all rows from a RecordBatch

4472ac8

github-actions Bot added Component: Python awaiting review Awaiting review labels Apr 8, 2025

koenvo changed the title ~~Fix IndexError when filtering all rows from a RecordBatch~~ GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty Apr 8, 2025

Fix code formatting

4322eea

raulcd reviewed Apr 8, 2025

View reviewed changes

Comment thread python/pyarrow/tests/arrow_44366.py Outdated

Comment thread python/pyarrow/acero.py Outdated

koenvo added 2 commits April 8, 2025 17:11

Simplify RecordBatch.filter check

feec82c

minor

51c4536

raulcd requested a review from AlenkaF April 8, 2025 15:39

AlenkaF reviewed Apr 9, 2025

View reviewed changes

Comment thread python/pyarrow/acero.py Outdated

Comment thread python/pyarrow/tests/test_acero.py Outdated

github-actions Bot added awaiting changes Awaiting changes awaiting committer review Awaiting committer review awaiting review Awaiting review and removed awaiting review Awaiting review awaiting committer review Awaiting committer review awaiting changes Awaiting changes labels Apr 9, 2025

Extend tests + make sure return type of filter equals input type

9db6ad6

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 9, 2025

Move tests to existing tests in test_compute

b5ef330

AlenkaF approved these changes Apr 9, 2025

View reviewed changes

raulcd approved these changes Apr 9, 2025

View reviewed changes

raulcd merged commit b1618e0 into apache:main Apr 9, 2025

raulcd removed the awaiting committer review Awaiting committer review label Apr 9, 2025

raulcd mentioned this pull request Apr 9, 2025

[Python][Acero] RecordBatch.filter on expression raises error if result set is empty #44366

Closed

koenvo deleted the bugfix/filter-record-batch branch April 9, 2025 11:05

WillAyd mentioned this pull request Apr 16, 2025

GH-36411: [C++][Python] Use meson-python for PyArrow build system #45854

Closed

AlenkaF mentioned this pull request Apr 17, 2025

[Python] Failing tests in python minimal builds #46174

Closed

Uh oh!

Conversation

koenvo commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented Apr 8, 2025

Uh oh!

Fokko commented Apr 8, 2025

Uh oh!

github-actions Bot commented Apr 8, 2025

Uh oh!

wgtmac commented Apr 8, 2025

Uh oh!

Uh oh!

Uh oh!

raulcd commented Apr 8, 2025

Uh oh!

AlenkaF left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AlenkaF left a comment

Choose a reason for hiding this comment

Uh oh!

koenvo commented Apr 9, 2025

Uh oh!

raulcd left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 9, 2025

Uh oh!

conbench-apache-arrow Bot commented Apr 9, 2025

Uh oh!

WillAyd commented Apr 16, 2025

Uh oh!

github-actions Bot commented Apr 16, 2025

Uh oh!

WillAyd commented Apr 16, 2025

Uh oh!

AlenkaF commented Apr 17, 2025

Uh oh!

raulcd commented Apr 17, 2025

Uh oh!

AlenkaF commented Apr 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

koenvo commented Apr 8, 2025 •

edited

Loading