Skip to content

GH-39131: [JS] Add at() for array like types #40712

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.

Sign up for GitHub

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

Jump to bottom
Closed
domoritz wants to merge 2 commits into from
Closed

GH-39131: [JS] Add at() for array like types #40712

domoritz wants to merge 2 commits into from

Conversation

@domoritz
Copy link
Member

@domoritz domoritz commented Mar 21, 2024 •
edited
Loading

Arrays in JavaScript have at instead of get so here we are making our arrays more consistent.

This PR still maintains support for get but deprecates it. We can remove get after one version of supporting both.

@domoritz domoritz requested a review from trxcllnt as a code owner March 21, 2024 14:51
@github-actions
Copy link

github-actions bot commented Mar 21, 2024

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

@domoritz domoritz mentioned this pull request Mar 21, 2024
Merged
@domoritz
Copy link
Member Author

domoritz commented Mar 21, 2024

@trxcllnt and I agreed offline to wrap indexes accesses so that any index is valid. The behavior in js is to only index -length ... length and return undefined otherwise but that seems less consistent than to just always wrap. Also, the types can be composer since we never return undefined.

@domoritz domoritz mentioned this pull request Mar 21, 2024
Merged
trxcllnt
trxcllnt requested changes Mar 21, 2024
View reviewed changes
Copy link
Contributor

@trxcllnt trxcllnt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

-1 for renaming get() to at(). I'm fine adding at(), but get() and set() are equivalent to accessing an element by subscript index, e.g. x = xs[0] and xs[0] = x.

We've tried to get rid of them in the past in favor of proxies, but they're still the fastest way to access elements besides enumerating the data directly (via iterators or arraybuffers).

@domoritz
Copy link
Member Author

domoritz commented Mar 21, 2024

I see. So you are saying we should keep the get visitor and just make at a method that calls get?

Isn't at as I implemented equivalent to get within the valid range of values? Do we want to keep get because it's slightly faster than at?

@domoritz domoritz mentioned this pull request Mar 22, 2024
Merged
@domoritz
Copy link
Member Author

domoritz commented Mar 22, 2024

Redone in #40730

@domoritz domoritz closed this Mar 22, 2024
domoritz added a commit that referenced this pull request Apr 16, 2024
@domoritz
GH-39131: [JS] Add at() for array like types (#40730)
7d18448 
Simpler version of #40712 that
preserves `get`.
* GitHub Issue: #39131
domoritz added a commit to domoritz/arrow that referenced this pull request Apr 17, 2024
@domoritz
apacheGH-39131: [JS] Add at() for array like types (apache#40730)
f28dd6d 
Simpler version of apache#40712 that
preserves `get`.
* GitHub Issue: apache#39131
raulcd pushed a commit that referenced this pull request Apr 29, 2024
@domoritz @raulcd
GH-39131: [JS] Add at() for array like types (#40730)
3d690b7 
Simpler version of #40712 that
preserves `get`.
* GitHub Issue: #39131
vibhatha pushed a commit to vibhatha/arrow that referenced this pull request May 25, 2024
@domoritz @vibhatha
apacheGH-39131: [JS] Add at() for array like types (apache#40730)
c2a0f67 
Simpler version of apache#40712 that
preserves `get`.
* GitHub Issue: apache#39131
kou pushed a commit to apache/arrow-js that referenced this pull request May 14, 2025
@domoritz @kou
GH-39131: [JS] Add at() for array like types (#40730)
3ded07b 
Simpler version of apache/arrow#40712 that
preserves `get`.
* GitHub Issue: #39131
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trxcllnt trxcllnt trxcllnt requested changes

Assignees

No one assigned

Labels

awaiting committer review Awaiting committer review Component: JavaScript

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@domoritz @trxcllnt