You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kylebarron (via GitHub)" <gi...@apache.org> on 2024/01/04 19:55:38 UTC
[PR] GH-39454: [JS] Support LargeList [arrow]
kylebarron opened a new pull request, #39457:
URL: https://github.com/apache/arrow/pull/39457
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### 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](https://github.com/kylebarron/arrow-js-ffi/blob/master/src/types.ts)) 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. https://github.com/apache/arrow/pull/39258
### Are these changes tested?
Yes
### Are there any user-facing changes?
Yes, new LargeList support.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39454: [JS] Support LargeList [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39457:
URL: https://github.com/apache/arrow/pull/39457#issuecomment-2910719756
:warning: GitHub issue #39454 **has no components**, please add labels for components.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39454: [JS] Support LargeList [arrow]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on PR #39457:
URL: https://github.com/apache/arrow/pull/39457#issuecomment-1879130250
> We have a safe method to cast (and throw an error if it doesn't work).
Can you point to an example using this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39454: [JS] Support LargeList [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #39457:
URL: https://github.com/apache/arrow/pull/39457#issuecomment-1879171467
For example https://github.com/apache/arrow/blob/38af25808e7826fb64265a78b2ed36b3882499f9/js/src/visitor/set.ts#L131.
I also just merged a pull request to remove getByteLength so you will need to merge/rebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39454: [JS] Support LargeList [arrow]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on PR #39457:
URL: https://github.com/apache/arrow/pull/39457#issuecomment-2031008379
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39454: [JS] Support LargeList [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #39457:
URL: https://github.com/apache/arrow/pull/39457#issuecomment-1879059939
Indexing should always be done with Numbers because arrays and buffers cannot be large than Number right now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39454: [JS] Support LargeList [arrow]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on PR #39457:
URL: https://github.com/apache/arrow/pull/39457#issuecomment-2033477662
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39454: [JS] Support LargeList [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #39457:
URL: https://github.com/apache/arrow/pull/39457#issuecomment-2033458305
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39454: [JS] Support LargeList [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic closed pull request #39457: GH-39454: [JS] Support LargeList
URL: https://github.com/apache/arrow/pull/39457
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39454: [JS] Support LargeList [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on code in PR #39457:
URL: https://github.com/apache/arrow/pull/39457#discussion_r1442926561
##########
js/src/data.ts:
##########
@@ -374,6 +375,13 @@ class MakeDataVisitor extends Visitor {
const { ['length']: length = valueOffsets.length - 1, ['nullCount']: nullCount = props['nullBitmap'] ? -1 : 0 } = props;
return new Data(type, offset, length, nullCount, [valueOffsets, undefined, nullBitmap], [child]);
}
+ public visitLargeList<T extends LargeList>(props: LargeListDataProps<T>) {
+ const { ['type']: type, ['offset']: offset = 0, ['child']: child } = props;
+ const nullBitmap = toUint8Array(props['nullBitmap']);
+ const valueOffsets = toInt32Array(props['valueOffsets']);
Review Comment:
```suggestion
const valueOffsets = toBigInt64Array(props['valueOffsets']);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39454: [JS] Support LargeList [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #39457:
URL: https://github.com/apache/arrow/pull/39457#issuecomment-4285259067
Closing this PR as the Javascript codebase has now moved to https://github.com/apache/arrow-js
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39454: [JS] Support LargeList [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #39457:
URL: https://github.com/apache/arrow/pull/39457#issuecomment-2020673869
Can you wrap up this pull request? It looks like it's pretty close and would nicely round out support in arrow js.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39454: [JS] Support LargeList [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #39457:
URL: https://github.com/apache/arrow/pull/39457#issuecomment-2033024895
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39454: [JS] Support LargeList [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39457:
URL: https://github.com/apache/arrow/pull/39457#issuecomment-1877679771
:warning: GitHub issue #39454 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39454: [JS] Support LargeList [arrow]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on PR #39457:
URL: https://github.com/apache/arrow/pull/39457#issuecomment-2033440636
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39454: [JS] Support LargeList [arrow]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on PR #39457:
URL: https://github.com/apache/arrow/pull/39457#issuecomment-1878944617
I am struggling a bit with casting between bigint and number.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org