Skip to content
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

ARROW-18247: [JS] fix: RangeError crash in Vector.toArray() #14587

Merged
merged 5 commits into from
Dec 11, 2022

Conversation

sarfata
Copy link
Contributor

@sarfata sarfata commented Nov 4, 2022

This fixes a bug which happens when a Vector has been created with multiple data blobs and at least one of them has been padded.

See https://observablehq.com/d/14488c116b338560 for a reproduction of the error and more details.

@github-actions
Copy link

github-actions bot commented Nov 4, 2022

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@sarfata sarfata changed the title fix: RangeError crash in Vector.toArray() ARROW-18247: [JS] fix: RangeError crash in Vector.toArray() Nov 4, 2022
@github-actions
Copy link

github-actions bot commented Nov 4, 2022

@github-actions
Copy link

github-actions bot commented Nov 4, 2022

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@kou kou requested review from domoritz and trxcllnt December 2, 2022 02:56
Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request. Please add a unit test so we make sure this issue doesn't surface again.

Copy link
Contributor

@trxcllnt trxcllnt left a comment

Choose a reason for hiding this comment

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

Nice catch, thanks for the fix @sarfata!

@sarfata
Copy link
Contributor Author

sarfata commented Dec 9, 2022

@domoritz @trxcllnt Thanks for your reviews.

I have added a unit test for my scenario. I also realized that my fix did not work when stride is not 1 so I have added a test (and fix) for that too.

I believe one of you will need to approve running the test workflow again before merging.

This fixes a bug which happens when a Vector has been created with
multiple data blobs and at least one of them has been padded.
@sarfata sarfata force-pushed the fix/vector-toarray-rangeerror branch from 1da239a to 8db45ee Compare December 9, 2022 19:29
@domoritz domoritz requested a review from trxcllnt December 9, 2022 21:26
@domoritz
Copy link
Member

domoritz commented Dec 9, 2022

Looks like a test is failing.

Screenshot 2022-12-09 at 17 21 24

@sarfata
Copy link
Contributor Author

sarfata commented Dec 9, 2022

@domoritz fixed.

🤦 I did some last minute cleanup and added this test to show exactly the key condition for this bug but ended up with a failing test. Sorry about this.

@domoritz domoritz merged commit 6cfe246 into apache:master Dec 11, 2022
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