-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add support to byte array decimal fields #97
Add support to byte array decimal fields #97
Conversation
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.
@YECHUNAN This looks great! While running locally I found a completely separate issue in the CI and build, so pushed up a fix for that, but it has nothing to do with your PR.
Thanks so much!
@@ -11,7 +11,7 @@ require('esbuild') | |||
}, { | |||
entryPoints: ['parquet.ts'], | |||
outfile: 'main.js', | |||
define: {"process.env.NODE_DEBUG": false, "process.env.NODE_ENV": "\"production\"", global: "window" }, | |||
define: {"process.env.NODE_DEBUG": "false", "process.env.NODE_ENV": "\"production\"", global: "window" }, |
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.
esbuild required.
@@ -61,6 +61,7 @@ Promise.all(targets.map(esbuild.build)) | |||
}) | |||
.catch(e => { | |||
console.error("Finished with errors: ", e.toString()); | |||
process.exit(1); |
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.
esbuild failure should be reported as such to stop CI
@@ -42,7 +42,7 @@ | |||
"buffer": "^6.0.3", | |||
"chai": "4.3.6", | |||
"core-js": "^3.22.5", | |||
"esbuild": "^0.14.47", | |||
"esbuild": "^0.19.2", |
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.
I did this to update esbuild to support the newer ts syntax used.
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.
Ran the tests and verified the two test files. Thanks for this work.
Problem
Address #91
Solution
When encountering such byte array represented "Decimal" fields, parse them into raw buffers.
Change summary:
Steps to Verify: