Conversation
|
|
domoritz
left a comment
There was a problem hiding this comment.
Thanks for the pull request. Maybe we can try to get this into arrow 15. The code freeze is Monday, though.
Co-authored-by: Dominik Moritz <domoritz@gmail.com>
|
I am struggling a bit with casting between bigint and number for indexing |
|
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). |
Can you point to an example using this? |
|
For example Line 131 in 38af258 I also just merged a pull request to remove getByteLength so you will need to merge/rebase. |
|
Can you wrap up this pull request? It looks like it's pretty close and would nicely round out support in arrow js. |
|
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 |
|
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? |
|
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! |
|
What docker tests? You can just run
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. |
I was just referring to the
With the new view types, completeness is a moving target 🥲 |
|
|
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.