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

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

Closed
wants to merge 2 commits into from

Conversation

domoritz
Copy link
Member

@domoritz domoritz commented Mar 21, 2024

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
Copy link

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

@domoritz
Copy link
Member Author

@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.

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.

-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

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
Copy link
Member Author

Redone in #40730

@domoritz domoritz closed this Mar 22, 2024
domoritz added a commit that referenced this pull request Apr 16, 2024
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
Simpler version of apache#40712 that
preserves `get`.
* GitHub Issue: apache#39131
raulcd pushed a commit that referenced this pull request Apr 29, 2024
Simpler version of #40712 that
preserves `get`.
* GitHub Issue: #39131
tolleybot pushed a commit to tmct/arrow that referenced this pull request May 2, 2024
vibhatha pushed a commit to vibhatha/arrow that referenced this pull request May 25, 2024
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.

2 participants