Parquetjs browser support #17

shannonwells · 2021-06-23T17:47:14Z

Problem

Need to be able to use parquetjs in browser, Fixes #178456668

with @enddynayn , @acruikshank , @aneyzberg

Solution Summary

Configuration for bundling parquetjs that works in browser, via esbuild
Add to build targets
Fix all the things that got broken
Example server showing how it can be used
Updates to README
Disable LZO and Brotli in browser (see notes)
Disable LZO in Node as a consequence of this attempt (see notes)

To verify:

Build parquetjs
Make sure tests all pass
Use the esbuild instructions to serve up the built parquetjs
Fire up the example server as shown in its README and play with it. Be sure to open your console. The only problems you should see are with anything marked "unsupported." Otherwise you should see the data in console.

Notes:

We attempted to get Brotli compression working in browser, almost got it, but hit a serious road bump with WebAssembly calls. There was a lot of work involved in the "almost", so since we may wish to get this working at a later date, rather than backing out of these changes and putting back the old brotli library, I have left these changes in just for NodeJS.
Turns out LZO is was already horribly broken, so it's disabled for NodeJS too.
Strangely, XXhash wasm library works just fine.
I had fixed a few other small bugs while trying to get this working, but I took the changes out of this PR and will submit later.

Example server working:

Use wasm-brotli: * Use async import to load wasm before loading compression.js * requires async loading to get the wasm instanc * bubble up all the asyncs * make the tests pass again Remove compiled bundle.js from repo Remove browserify tsconfig

…uetjs module

disable LZO completely due to overrun error

shannonwells · 2021-06-30T18:38:26Z

lib/bloomFilterIO/bloomFilterReader.ts

-      },
-    } = columnChunk;
-    return bloomFilterOffsetBuffer;
+    if (!columnChunk.column.meta_data.bloom_filter_offset) {


the previous form was causing an error in the browser when meta_data was undefined.

This should cause an error everywhere if columnChunk.column.meta_data is undefined. Did you mean to say when bloom_filter_offset is undefined? Also this should be redundant to the optional chain below unless you're really checking for falsey rather than undefined here. I would drop it.

Now that I think it was actually bloom_filter_offset, and somehow the case I was looking at was getting through the if condition.

acruikshank

This is great work. I left a few non-blocking requests.

acruikshank · 2021-06-30T19:29:36Z

webpack.config.js

@@ -0,0 +1,99 @@
+/**
+ * Left here in case esbuild stops working for us when we try to re-enable


This comment is good, but this should be renamed webpack.config.js.example or all the code in this file should be commented out if we're not actually using it. It's likely a future developer will land in the middle of this file by searching or just miss the comment and get really confused about how the module is built. It might also be good to name check the actual build system (esbuild?) in this comment.

examples/server/app.js

acruikshank · 2021-06-30T19:43:48Z

lib/bloomFilterIO/bloomFilterReader.ts

-      },
-    } = columnChunk;
-    return bloomFilterOffsetBuffer;
+    if (!columnChunk.column.meta_data.bloom_filter_offset) {


This should cause an error everywhere if columnChunk.column.meta_data is undefined. Did you mean to say when bloom_filter_offset is undefined? Also this should be redundant to the optional chain below unless you're really checking for falsey rather than undefined here. I would drop it.

lib/compression.js

acruikshank · 2021-06-30T19:52:43Z

lib/reader.js

@@ -603,13 +601,12 @@ class ParquetEnvelopeReader {
    if (colChunk.meta_data.dictionary_page_offset) {
      const offset = +colChunk.meta_data.dictionary_page_offset;
      const size = Math.min(+this.fileSize - offset, this.default_dictionary_size);
-      dictionary = this.read(offset, size, colChunk.file_path).then(buffer => decodePage({offset: 0, buffer, size: buffer.length}, opts).dictionary);
+      const buffer = await this.read(offset, size, colChunk.file_path)
+      const dict = await decodePage({offset: 0, buffer, size: buffer.length}, opts)


Is is necessary to read and decode the dictionary if opts.dictionary is already set? It'd be nice to avoid it if we're just going to throw it away, but maybe it's required to start the next read in the right place.

It's not absolutely required, it just made it a little more readable.

test/integration.js

* in reader.js, don't use throwaway variables, use nested "thens" * rename webpack.config.js to webpack.config.js.example

…t generated files from being checked in

enddynayn and others added 13 commits June 18, 2021 14:15

draft

6fa5043

static file server

7cace4c

esbuild

d30bada

remove static file server stuff

0063373

WIP it

4f1c269

WIP saving webpack configuration

9685320

Webpack config file

5a882bd

Use wasm-brotli: * Use async import to load wasm before loading compression.js * requires async loading to get the wasm instanc * bubble up all the asyncs * make the tests pass again Remove compiled bundle.js from repo Remove browserify tsconfig

Working configuration that compiles, serves and allows import of parq…

0d85f65

…uetjs module

parquet files loading

a1b52ad

disallow lzo and brotli compression temporarily

e1c0809

getting a build working and passing tests

4bcbea9

disable LZO completely due to overrun error

WIP debugging

8503b3d

tests passing

c63f60f

shannonwells force-pushed the for-browser-sew branch from 274eedb to c63f60f Compare June 29, 2021 23:49

shannonwells added 4 commits June 29, 2021 17:06

more prep for publishing

9d767ed

some basic cleanup for PR

427f496

don't fix things unrelated to browser stuff

9ee84f7

run npm install

8d7641b

shannonwells changed the title ~~WIP For browser sew~~ Parquetjs browser support Jun 30, 2021

improved example, comment

d2de3c7

shannonwells commented Jun 30, 2021

View reviewed changes

don't check in whitespace changes

b4a609c

shannonwells marked this pull request as ready for review June 30, 2021 19:19

shannonwells requested review from acruikshank and enddynayn June 30, 2021 19:19

shannonwells assigned aneyzberg Jun 30, 2021

acruikshank approved these changes Jun 30, 2021

View reviewed changes

shannonwells added 3 commits June 30, 2021 13:18

* remove commented-out LZO code, leave link to Issue

ffec956

* in reader.js, don't use throwaway variables, use nested "thens" * rename webpack.config.js to webpack.config.js.example

check in example files

82703f6

reminder in example server

751c52f

reminder in example server, put back *.parquet in gitignore to preven…

00d8256

…t generated files from being checked in

enddynayn approved these changes Jun 30, 2021

View reviewed changes

shannonwells merged commit b35942b into main Jun 30, 2021

shannonwells deleted the for-browser-sew branch June 30, 2021 22:29

This was referenced Jul 1, 2021

fix bug caused by side-effect of change in bloomFilterReader. regen package-lock #19

Merged

Use parquetjs v1.1.0 LibertyDSNP/eth-sdk-ts#82

Merged

wesbiggs mentioned this pull request Feb 17, 2023

DIP-225/DIP-227 Graph and User Data Operations LibertyDSNP/spec#233

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parquetjs browser support #17

Parquetjs browser support #17

shannonwells commented Jun 23, 2021 •

edited

Loading

shannonwells Jun 30, 2021

acruikshank Jun 30, 2021

shannonwells Jun 30, 2021

acruikshank left a comment

acruikshank Jun 30, 2021

acruikshank Jun 30, 2021

acruikshank Jun 30, 2021

shannonwells Jun 30, 2021

		@@ -0,0 +1,99 @@
		/**
		* Left here in case esbuild stops working for us when we try to re-enable

Parquetjs browser support #17

Parquetjs browser support #17

Conversation

shannonwells commented Jun 23, 2021 • edited Loading

Problem

Solution Summary

To verify:

Example server working:

shannonwells Jun 30, 2021

Choose a reason for hiding this comment

acruikshank Jun 30, 2021

Choose a reason for hiding this comment

shannonwells Jun 30, 2021

Choose a reason for hiding this comment

acruikshank left a comment

Choose a reason for hiding this comment

acruikshank Jun 30, 2021

Choose a reason for hiding this comment

acruikshank Jun 30, 2021

Choose a reason for hiding this comment

acruikshank Jun 30, 2021

Choose a reason for hiding this comment

shannonwells Jun 30, 2021

Choose a reason for hiding this comment

shannonwells commented Jun 23, 2021 •

edited

Loading