-
Notifications
You must be signed in to change notification settings - Fork 6.5k
[Data] Fixing PyArrow overflow handling #53971
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
# Remaining 2 go into the second one | ||
assert result.chunks[1].nbytes == sum([c.nbytes for c in input_.chunks[14:]]) | ||
|
||
@pytest.mark.parametrize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice coverage
@@ -361,8 +362,9 @@ def test_combine_chunked_array_small( | |||
expected_output.equals(result) | |||
|
|||
|
|||
def test_combine_chunked_array_large(): | |||
"""Verifies `combine_chunked_array` on arrays > 2 GiB""" | |||
def test_combine_chunked_fixed_width_array_large(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will go good to add coverage for all _VARIABLE_WIDTH_INT32_OFFSET_PA_TYPE_PREDICATES
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does below (this is for fixed-width types)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add PR description?
Is the new behavior always produce one single chunk instead of multiple 2GB chunks?
…32 offsets overflows (irrelevant for fixed-width arrays) Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
ced6071
to
5945253
Compare
Updated description, that should hopefully answer that question, LMK |
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
<!-- Thank you for your contribution! Please review https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before opening a pull request. --> <!-- Please add a reviewer to the assignee section when you create a PR. If you don't have the access to it, we will shortly find a reviewer and assign them to your PR. --> ## Why are these changes needed? Current int32 offsets overflow handling is missing appropriate handling of the fixed-width types (any types not involving arbitrary length types like strings, lists, binary, etc). As a result this has been incorrectly enforcing the 2Gb limit on blocks that actually don't have that limit. This PR addresses that. Changes 1. Make sure overflow handling only applied to variable-width types 2. Fixed chunking method to prevent adding empty chunks 3. Updated tests ## Related issue number <!-- For example: "Closes ray-project#1234" --> ## Checks - [ ] I've signed off every commit(by using the -s flag, i.e., `git commit -s`) in this PR. - [ ] I've run `scripts/format.sh` to lint the changes in this PR. - [ ] I've included any doc changes needed for https://docs.ray.io/en/master/. - [ ] I've added any new APIs to the API Reference. For example, if I added a method in Tune, I've added it in `doc/source/tune/api/` under the corresponding `.rst` file. - [ ] I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/ - Testing Strategy - [ ] Unit tests - [ ] Release tests - [ ] This PR is not tested :( --------- Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
Why are these changes needed?
Current int32 offsets overflow handling is missing appropriate handling of the fixed-width types (any types not involving arbitrary length types like strings, lists, binary, etc).
As a result this has been incorrectly enforcing the 2Gb limit on blocks that actually don't have that limit. This PR addresses that.
Changes
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.