You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Lukedowling444 (via GitHub)" <gi...@apache.org> on 2024/10/16 03:06:11 UTC
[PR] GH-44232: [Python] Validate __arrow_c_array__ length for scalar construction [arrow]
Lukedowling444 opened a new pull request, #44434:
URL: https://github.com/apache/arrow/pull/44434
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). 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](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) 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}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
In the current implementation of the pyarrow.scalar constructor, when an input has the __arrow_c_array__ interface, there is no check to ensure the length of the array is exactly 1. This can lead to unexpected behavior or invalid data being accepted for scalar construction.
This change introduces a validation step that checks whether the input array has a length of 1 when using the __arrow_c_array__ interface, ensuring that only valid inputs are processed. This improves the reliability of Arrow when handling data from multiple libraries.
An existing workaround involves calling pa.array(other_scalar).slice(0, 1) to achieve the same result, but this change eliminates the need for such a workaround.
Looking to close #44232
### What changes are included in this PR?
A validation check was added inside the pyarrow.scalar constructor to ensure that when an input with the __arrow_c_array__ interface is provided, it must be a length-1 array.
The constructor now raises a ValueError if this condition is not met.
### Are these changes tested?
Yes, the project builds successfully, and all existing test cases pass without any issues.
Additionally, I have included new test cases to specifically check the behavior of this change in python/scripts/test_scalar.py. These tests ensure that the pyarrow.scalar constructor raises a ValueError when a non-length-1 array is provided and continues to work correctly for valid inputs.
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
### Are there any user-facing changes?
No user-facing changes are introduced, and this validation should not affect any workflows where inputs to pyarrow.scalar are already correct.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
GitHub Issue: https://github.com/apache/arrow/issues/44232
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44232: [Python] Validate __arrow_c_array__ length for scalar construction [arrow]
Posted by "Isaac7777-cpu-school (via GitHub)" <gi...@apache.org>.
Isaac7777-cpu-school commented on PR #44434:
URL: https://github.com/apache/arrow/pull/44434#issuecomment-2422313146
@lukedowling44 This looks perfect now and have fixed the previous issue. I would like to ask why most of the statements inside the if-clause is quite similar to the execution statements in the following.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44232: [Python] Validate __arrow_c_array__ length for scalar construction [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #44434:
URL: https://github.com/apache/arrow/pull/44434#issuecomment-2694966567
cc @AlenkaF
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44232: [Python] Validate __arrow_c_array__ length for scalar construction [arrow]
Posted by "piratepanda805 (via GitHub)" <gi...@apache.org>.
piratepanda805 commented on PR #44434:
URL: https://github.com/apache/arrow/pull/44434#issuecomment-2415940893
> Hi @lukedowling44. This looks awsome. However, I think you might have accidentally commited your build folder (`\dist`) and your virtual environment (`\pyarrow-dev`) as well. I think these files are better not get committed to the git repository.
To clarify, make sure to only commit the test_scalar.py and scalar.pxi files.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44232: [Python] Validate __arrow_c_array__ length for scalar construction [arrow]
Posted by "Lukedowling444 (via GitHub)" <gi...@apache.org>.
Lukedowling444 closed pull request #44434: GH-44232: [Python] Validate __arrow_c_array__ length for scalar construction
URL: https://github.com/apache/arrow/pull/44434
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44232: [Python] Validate __arrow_c_array__ length for scalar construction [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #44434: GH-44232: [Python] Validate __arrow_c_array__ length for scalar construction
URL: https://github.com/apache/arrow/pull/44434
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44232: [Python] Validate __arrow_c_array__ length for scalar construction [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #44434:
URL: https://github.com/apache/arrow/pull/44434#discussion_r1979184881
##########
python/scripts/test_scalar.py:
##########
@@ -0,0 +1,28 @@
+import pyarrow as pa
Review Comment:
The test should be in https://github.com/apache/arrow/blob/main/python/pyarrow/tests/test_scalars.py.
Can you also check the existing tests for array constructors and do similar?
https://github.com/apache/arrow/blob/6d75b0e6e1b049089fb11955103a44bbc495bd92/python/pyarrow/tests/test_array.py#L3611-L3616
https://github.com/apache/arrow/blob/6d75b0e6e1b049089fb11955103a44bbc495bd92/python/pyarrow/tests/test_array.py#L3628-L3637
##########
python/pyarrow/scalar.pxi:
##########
@@ -1235,6 +1235,13 @@ def scalar(value, type=None, *, from_pandas=None, MemoryPool memory_pool=None):
ty = ensure_type(type)
options.type = ty.sp_type
+ cdef shared_ptr[CArray] c_array
+
+ if hasattr(value, "__arrow_c_array__"):
+ c_array = pyarrow_unwrap_array(value.__arrow_c_array__())
+ if c_array.get().length() != 1:
+ raise ValueError("Expected a length-1 array for scalar construction")
+
Review Comment:
Can you look at how the array constructor handles arrays that implement the Arrow PyCapsule Protocol and try to do similar? https://github.com/apache/arrow/blob/6d75b0e6e1b049089fb11955103a44bbc495bd92/python/pyarrow/array.pxi#L268-L279
Note that you have to return an object which should be a Scalar. To retrieve the scalar from pyarrow array of length 1 see the code bellow, at the end of this method.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44232: [Python] Validate __arrow_c_array__ length for scalar construction [arrow]
Posted by "Isaac7777-cpu-school (via GitHub)" <gi...@apache.org>.
Isaac7777-cpu-school commented on PR #44434:
URL: https://github.com/apache/arrow/pull/44434#issuecomment-2423730957
@Lukedowling444 Nice, all the dead codes are removed. I think it is pretty much ready for the PR. However, just a minor thing and this may not make sense as much - why do we need another local variable for the carray rather than directly put it in the if condition? However, since this is really just a styling issue, please also consult with the maintainer for this as well?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44232: [Python] Validate __arrow_c_array__ length for scalar construction [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #44434:
URL: https://github.com/apache/arrow/pull/44434#issuecomment-4011146325
Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44232: [Python] Validate __arrow_c_array__ length for scalar construction [arrow]
Posted by "Isaac7777-cpu-school (via GitHub)" <gi...@apache.org>.
Isaac7777-cpu-school commented on PR #44434:
URL: https://github.com/apache/arrow/pull/44434#issuecomment-2415926335
Hi @lukedowling44. This looks awsome. However, I think you might have accidentally commited your build folder (`\dist`) and your virtual environment (`\pyarrow-dev`) as well. I think these files are better not get committed to the git repository.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44232: [Python] Validate __arrow_c_array__ length for scalar construction [arrow]
Posted by "Lukedowling444 (via GitHub)" <gi...@apache.org>.
Lukedowling444 commented on PR #44434:
URL: https://github.com/apache/arrow/pull/44434#issuecomment-2439823696
@Isaac7777-cpu-school Thanks for the feedback. Cython doesn’t allow cdef statements directly within conditional logic or non-function scopes, so I had to define it before the condition.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44232: [Python] Validate __arrow_c_array__ length for scalar construction [arrow]
Posted by "Lukedowling444 (via GitHub)" <gi...@apache.org>.
Lukedowling444 commented on PR #44434:
URL: https://github.com/apache/arrow/pull/44434#issuecomment-2423391533
@Isaac7777-cpu-school You are right, I was repeating code unnecessarily. I have made changes and pushed again.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44232: [Python] Validate __arrow_c_array__ length for scalar construction [arrow]
Posted by "samir-nasibli (via GitHub)" <gi...@apache.org>.
samir-nasibli commented on code in PR #44434:
URL: https://github.com/apache/arrow/pull/44434#discussion_r1836632205
##########
python/scripts/test_scalar.py:
##########
@@ -0,0 +1,28 @@
+import pyarrow as pa
+
+# Class with valid __arrow_c_array__ method that returns a length-1 array
+class CustomCapsule:
+ def __arrow_c_array__(self):
+ return pa.array([42])
+
+# Test case to check that a scalar can be created from an object with __arrow_c_array__
+# Adjusting the test to expect the current behavior, which is an ArrowInvalid error
+def test_scalar_with_capsule():
+ capsule = CustomCapsule()
+ try:
+ pa.scalar(capsule)
+ except pa.lib.ArrowInvalid as e:
+ assert "did not recognize Python value type" in str(e)
+
+# Class with invalid __arrow_c_array__ method that returns an array of length > 1
+# Adjusting the test to expect the current behavior, which is an ArrowInvalid error
+def test_scalar_invalid_length():
+ class InvalidCapsule:
+ def __arrow_c_array__(self):
+ return pa.array([1, 2])
+
+ capsule = InvalidCapsule()
+ try:
+ pa.scalar(capsule)
+ except pa.lib.ArrowInvalid as e:
+ assert "did not recognize Python value type" in str(e)
Review Comment:
```suggestion
assert "did not recognize Python value type" in str(e)
```
please add new line at the end of 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org