-
Notifications
You must be signed in to change notification settings - Fork 19
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
Streams implementation #296
Changes from all commits
2da4734
ab7c571
7ad666b
359922e
18f9cbb
4690ad9
7c598f3
058f606
0dcc0b2
8051f22
35fac83
2a2ab3d
bdf582d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,9 @@ wasm-pack build \ | |
--out-dir tmp_build/node \ | ||
--out-name arrow1 \ | ||
--target nodejs \ | ||
$FLAGS | ||
--features async \ | ||
$FLAGS & | ||
[ -n "$CI" ] && wait; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is awesome; it parallelizes each build? Should we do this for arrow2 as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, turning the parallel behaviour off in CI took a bit of bash-fu (conditionally backgrounding a command isn't possible, but nobody said anything about conditionally running Not much point for arrow2 (the wasm-opt step for arrow2 builds is quite short, while the compile step is highly parallelized, so trying to run that in parallel ended up slowing things down). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder why wasm-opt would be so much slower for arrow1. I don't think this used to be the case; I only recently noticed it taking longer |
||
|
||
# Build web version into tmp_build/esm | ||
echo "Building arrow-rs esm" | ||
|
@@ -28,7 +30,9 @@ wasm-pack build \ | |
--out-dir tmp_build/esm \ | ||
--out-name arrow1 \ | ||
--target web \ | ||
$FLAGS | ||
--features async \ | ||
$FLAGS & | ||
[ -n "$CI" ] && wait; | ||
|
||
# Build bundler version into tmp_build/bundler | ||
echo "Building arrow-rs bundler" | ||
|
@@ -37,7 +41,9 @@ wasm-pack build \ | |
--out-dir tmp_build/bundler \ | ||
--out-name arrow1 \ | ||
--target bundler \ | ||
$FLAGS | ||
--features async \ | ||
$FLAGS & | ||
wait | ||
|
||
###################################### | ||
# ARROW 2 turn on the feature manually | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this require a Node 18+ for users as well? Or is this just for our tests? Is there a shim/polyfill that users can use for pre-18 versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, turns out the ReadableStream constructor only showed up in Node 18. There is indeed a poly/ponyfill, written by a familar face: MattiasBuelens/web-streams-polyfill.
That does bring up a good point - overall README level documentation, and doc-strings for the new exports. On that note, the codeblocks embedded in rust doc comments, do those show up with syntax highlighting for embedded code fragments (a bit like jsdocs) in editors for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah they show up in editors and on the docs site https://kylebarron.dev/parquet-wasm/modules/bundler_arrow1.html#readParquet