Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
WillAyd commented on PR #46057: URL: https://github.com/apache/arrow/pull/46057#issuecomment-2811068182 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
AlenkaF commented on PR #46057: URL: https://github.com/apache/arrow/pull/46057#issuecomment-281345 > or mark them as `@pytest.mark.acero` True! I already opened a PR: https://github.com/apache/arrow/pull/46175. Can change to adding a mark if there will be a preference for it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
raulcd commented on PR #46057: URL: https://github.com/apache/arrow/pull/46057#issuecomment-2812212645 or mark them as `@pytest.mark.acero` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
AlenkaF commented on PR #46057: URL: https://github.com/apache/arrow/pull/46057#issuecomment-2812157272 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! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
WillAyd commented on PR #46057: URL: https://github.com/apache/arrow/pull/46057#issuecomment-2811039063 @github-actions crossbow submit -g python -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
github-actions[bot] commented on PR #46057: URL: https://github.com/apache/arrow/pull/46057#issuecomment-2811046768 Revision: b5ef33038f5bbb6b848edf600a23072d9082c1b9 Submitted crossbow builds: [ursacomputing/crossbow @ actions-85876e4ee6](https://github.com/ursacomputing/crossbow/branches/all?query=actions-85876e4ee6) |Task|Status| ||--| |example-python-minimal-build-fedora-conda|[](https://github.com/ursacomputing/crossbow/actions/runs/14504182772/job/40690374094)| |example-python-minimal-build-ubuntu-venv|[](https://github.com/ursacomputing/crossbow/actions/runs/14504182883/job/40690374677)| |test-conda-python-3.10|[](https://github.com/ursacomputing/crossbow/actions/runs/14504182949/job/40690375065)| |test-conda-python-3.10-hdfs-2.9.2|[](https://github.com/ursacomputing/crossbow/actions/runs/14504183016/job/40690375360)| |test-conda-python-3.10-hdfs-3.2.1|[](https://github.com/ursacomputing/crossbow/actions/runs/14504183084/job/40690376569)| |test-conda-python-3.10-pandas-latest-numpy-latest|[](https://github.com/ursacomputing/crossbow/actions/runs/14504182924/job/40690374714)| |test-conda-python-3.11|[](https://github.com/ursacomputing/crossbow/actions/runs/14504183057/job/40690376056)| |test-conda-python-3.11-dask-latest|[](https://github.com/ursacomputing/crossbow/actions/runs/14504182805/job/40690374076)| |test-conda-python-3.11-dask-upstream_devel|[](https://github.com/ursacomputing/crossbow/actions/runs/14504183028/job/40690375551)| |test-conda-python-3.11-hypothesis|[](https://github.com/ursacomputing/crossbow/actions/runs/14504182969/job/40690375267)| |test-conda-python-3.11-pandas-latest-numpy-1.26|[](https://github.com/ursacomputing/crossbow/actions/runs/14504182944/job/40690375036)| |test-conda-python-3.11-pandas-latest-numpy-latest|[](https://github.com/ursacomputing/crossbow/actions/runs/14504183100/job/40690376599)| |test-conda-python-3.11-pandas-nightly-numpy-nightly|[](https://github.com/ursacomputing/crossbow/actions/runs/14504182951/job/40690375052)| |test-conda-python-3.11-pandas-upstream_devel-numpy-nightly|[](https://github.com/ursacomputing/crossbow/actions/runs/14504183233/job/40690377299)| |test-conda-python-3.11-spark-master|[](https://github.com/ursacomputing/crossbow/actions/runs/14504183409/job/40690377278)| |test-cond
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
raulcd commented on code in PR #46057: URL: https://github.com/apache/arrow/pull/46057#discussion_r2033372817 ## python/pyarrow/tests/arrow_44366.py: ## @@ -0,0 +1,28 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# This file is called from a test in test_pandas.py. Review Comment: This test is not called from test_pandas. The current tests with `arrow_xxx.py` are specifically invoked. Could you add a test in `test_acero.py` instead of creating this file? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
conbench-apache-arrow[bot] commented on PR #46057: URL: https://github.com/apache/arrow/pull/46057#issuecomment-2790851145 After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit b1618e0af67c88cce4b00cf94c4636fdfaf1227d. There were no benchmark performance regressions. 🎉 The [full Conbench report](https://github.com/apache/arrow/runs/40278504737) has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
koenvo commented on code in PR #46057:
URL: https://github.com/apache/arrow/pull/46057#discussion_r2034836989
##
python/pyarrow/acero.py:
##
@@ -374,7 +374,7 @@ def _filter_table(table, expression):
Declaration("filter", options=FilterNodeOptions(expression))
])
result = decl.to_table(use_threads=True)
-if is_batch:
+if is_batch and result.num_rows > 0:
Review Comment:
Ah I should have updated the docstring. The original implementation (without
my change) returns a RecordBatch when the input is a RecordBatch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
raulcd merged PR #46057: URL: https://github.com/apache/arrow/pull/46057 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
github-actions[bot] commented on PR #46057: URL: https://github.com/apache/arrow/pull/46057#issuecomment-2789313071 :warning: GitHub issue #44366 **has been automatically assigned in GitHub** to PR creator. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
koenvo commented on PR #46057: URL: https://github.com/apache/arrow/pull/46057#issuecomment-2789133314 Thank you all so much for the guidance! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
AlenkaF commented on code in PR #46057: URL: https://github.com/apache/arrow/pull/46057#discussion_r2034858244 ## python/pyarrow/tests/test_acero.py: ## @@ -115,6 +115,14 @@ def test_filter(table_source): FilterNodeOptions(None) +def test_filter_all_rows(): Review Comment: Ah, I see there are existing tests in `test_compute.py`: https://github.com/apache/arrow/blob/a5b2574208587464008127bbc85619562e6e85dd/python/pyarrow/tests/test_compute.py#L1399-L1435 Can we move it there? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
AlenkaF commented on code in PR #46057:
URL: https://github.com/apache/arrow/pull/46057#discussion_r2034862879
##
python/pyarrow/acero.py:
##
@@ -374,7 +374,7 @@ def _filter_table(table, expression):
Declaration("filter", options=FilterNodeOptions(expression))
])
result = decl.to_table(use_threads=True)
-if is_batch:
+if is_batch and result.num_rows > 0:
Review Comment:
> Will look into the C++ docs.
Found nothing that would help us here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
raulcd commented on code in PR #46057:
URL: https://github.com/apache/arrow/pull/46057#discussion_r2034824481
##
python/pyarrow/acero.py:
##
@@ -374,7 +374,7 @@ def _filter_table(table, expression):
Declaration("filter", options=FilterNodeOptions(expression))
])
result = decl.to_table(use_threads=True)
-if is_batch:
+if is_batch and result.num_rows > 0:
Review Comment:
@AlenkaF I might have miss-understood here but shouldn't this return a Table
on that case (as per docstring?). Otherwise we should update the docstring to
specify that we can return a RecordBatch.
The initial commit returned a RecordBatch on that case, see:
https://github.com/apache/arrow/pull/46057/commits/4472ac84067e9fd33061bca2ea7cb143afdf86d4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
AlenkaF commented on code in PR #46057:
URL: https://github.com/apache/arrow/pull/46057#discussion_r2034844303
##
python/pyarrow/acero.py:
##
@@ -374,7 +374,7 @@ def _filter_table(table, expression):
Declaration("filter", options=FilterNodeOptions(expression))
])
result = decl.to_table(use_threads=True)
-if is_batch:
+if is_batch and result.num_rows > 0:
Review Comment:
Yeah, it is a bit confusing. Looking at the `Table.filter` method:
https://github.com/apache/arrow/blob/a5b2574208587464008127bbc85619562e6e85dd/python/pyarrow/table.pxi#L2199-L2219
and the `is_batch` variable defined in `_filter_table` I would think the
type should stay the same.
Will look into the C++ docs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
AlenkaF commented on code in PR #46057:
URL: https://github.com/apache/arrow/pull/46057#discussion_r2034684720
##
python/pyarrow/acero.py:
##
@@ -374,7 +374,7 @@ def _filter_table(table, expression):
Declaration("filter", options=FilterNodeOptions(expression))
])
result = decl.to_table(use_threads=True)
-if is_batch:
+if is_batch and result.num_rows > 0:
Review Comment:
With this check we will return an empty `Table` instead of an empty
`RecordBatch`. We might use try and catch or add an additional else so that in
case the the input is a `RecordBatch` and the filtering result is empty, we
still return an empty `RecordBatch`.
##
python/pyarrow/tests/test_acero.py:
##
@@ -115,6 +115,14 @@ def test_filter(table_source):
FilterNodeOptions(None)
+def test_filter_all_rows():
Review Comment:
Can we add a check that the result is a `RecordBatch`? And maybe a test with
the `Table` as an input for the empty filtering result so we are sure not to
break anything there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
AlenkaF commented on code in PR #46057: URL: https://github.com/apache/arrow/pull/46057#discussion_r2034713347 ## python/pyarrow/acero.py: ## @@ -375,7 +396,13 @@ def _filter_table(table, expression): ]) result = decl.to_table(use_threads=True) if is_batch: -result = result.combine_chunks().to_batches()[0] +batches = result.combine_chunks().to_batches() +result = ( +batches[0] +if batches +else _empty_record_batch_from_schema(result.schema) Review Comment: The logic here might not be wrong (I think you are correct about returning same type as input type). We might just make it a bit simpler. cc @raulcd -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
koenvo commented on code in PR #46057: URL: https://github.com/apache/arrow/pull/46057#discussion_r2033424510 ## python/pyarrow/acero.py: ## @@ -375,7 +396,13 @@ def _filter_table(table, expression): ]) result = decl.to_table(use_threads=True) if is_batch: -result = result.combine_chunks().to_batches()[0] +batches = result.combine_chunks().to_batches() +result = ( +batches[0] +if batches +else _empty_record_batch_from_schema(result.schema) Review Comment: Yes. Makes sense. I changed it like this to keep return type same as input type. Changed it in latest commit. ## python/pyarrow/tests/arrow_44366.py: ## @@ -0,0 +1,28 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# This file is called from a test in test_pandas.py. Review Comment: Changed it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
wgtmac commented on PR #46057: URL: https://github.com/apache/arrow/pull/46057#issuecomment-2786663471 Thanks for the fix! It looks reasonable to me. However, I'm not familiar with the convention here. Perhaps @jorisvandenbossche @pitrou can help. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
raulcd commented on PR #46057: URL: https://github.com/apache/arrow/pull/46057#issuecomment-2786734418 Thanks for the PR! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
raulcd commented on code in PR #46057: URL: https://github.com/apache/arrow/pull/46057#discussion_r2033388707 ## python/pyarrow/acero.py: ## @@ -375,7 +396,13 @@ def _filter_table(table, expression): ]) result = decl.to_table(use_threads=True) if is_batch: -result = result.combine_chunks().to_batches()[0] +batches = result.combine_chunks().to_batches() +result = ( +batches[0] +if batches +else _empty_record_batch_from_schema(result.schema) Review Comment: Do we really need the auxiliary function? Couldn't we just return result (this is already an empty table) ## python/pyarrow/tests/arrow_44366.py: ## @@ -0,0 +1,28 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# This file is called from a test in test_pandas.py. Review Comment: This test is not called from pandas. The current tests with `arrow_xxx.py` are specifically invoked. Could you add a test in `test_acero.py` instead of creating this file? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-44366: [Python][Acero] RecordBatch.filter on expression raises error if result set is empty [arrow]
github-actions[bot] commented on PR #46057: URL: https://github.com/apache/arrow/pull/46057#issuecomment-2786641502 :warning: GitHub issue #44366 **has been automatically assigned in GitHub** to PR creator. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
