Skip to content

GH-39454: [JS] Support LargeList #39457

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

kylebarron
Copy link
Contributor

@kylebarron kylebarron commented Jan 4, 2024

Rationale for this change

Support large list type in JS bindings.

With the addition of LargeBinary and LargeUtf8, I'd like to get LargeList in as well, so that I don't have to special case any large-offset types (as I currently do) when handling FFI to Arrow JS

cc @domoritz @trxcllnt

What changes are included in this PR?

Mimics the previous PR to add large binary support. #39258

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, new LargeList support.

Copy link

github-actions bot commented Jan 4, 2024

⚠️ GitHub issue apache/arrow-js#70 has been automatically assigned in GitHub to PR creator.

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.

Thanks for the pull request. Maybe we can try to get this into arrow 15. The code freeze is Monday, though.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jan 5, 2024
Co-authored-by: Dominik Moritz <[email protected]>
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 5, 2024
@kylebarron
Copy link
Contributor Author

kylebarron commented Jan 5, 2024

I am struggling a bit with casting between bigint and number for indexing

@domoritz
Copy link
Member

domoritz commented Jan 5, 2024

Indexing should always be done with Numbers because arrays and buffers cannot be large than Number right now. We have a safe method to cast (and throw an error if it doesn't work).

@kylebarron
Copy link
Contributor Author

We have a safe method to cast (and throw an error if it doesn't work).

Can you point to an example using this?

@domoritz
Copy link
Member

domoritz commented Jan 5, 2024

For example

const x = bigIntToNumber(valueOffsets[index]);
.

I also just merged a pull request to remove getByteLength so you will need to merge/rebase.

@domoritz
Copy link
Member

Can you wrap up this pull request? It looks like it's pretty close and would nicely round out support in arrow js.

@kylebarron
Copy link
Contributor Author

I took a quick look back at this and it feels like there's quite a few places where offsets are used and are assumed to be Number, and it's hard to tell how much work would be left

@domoritz
Copy link
Member

domoritz commented Apr 2, 2024

I already implemented support for large offsets (e.g. for large binary and large utf-8). You should be able to copy what I did there. Is there anything specific you are looking at?

@kylebarron
Copy link
Contributor Author

kylebarron commented Apr 3, 2024

It seems like there are a whole lot of errors in the docker tests.

LargeList support is a "nice to have" for me, but ultimately I just don't have a use case that really needs this. In lonboard I'm constructing the data myself and can ensure I have i32 offsets. In parquet-wasm, arrow-js-ffi, and geoarrow-wasm, I can ensure the user has i32 offsets. i64 offsets would mostly support polars integration I suppose.

Sorry but this is kinda low on my long list of tasks. Much higher is prototyping lonboard-mosaic integration!

@domoritz
Copy link
Member

domoritz commented Apr 3, 2024

What docker tests? You can just run yarn test -t src locally.

LargeList support is a "nice to have" for me, but ultimately I just don't have a use case that really needs this.

Yeah, it's mostly about completeness for me. I'll see whether I can put it on my queue to wrap up this pull request.

@kylebarron
Copy link
Contributor Author

kylebarron commented Apr 3, 2024

What docker tests? You can just run yarn test -t src locally.

I was just referring to the Execute Docker Build step of CI that's the name of the failing stage.

Yeah, it's mostly about completeness for me

With the new view types, completeness is a moving target 🥲

Copy link

⚠️ GitHub issue #39454 has no components, please add labels for components.

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.

[JS] Support LargeList
2 participants