Skip to content
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

Conversation

YECHUNAN
Copy link

@YECHUNAN YECHUNAN commented Aug 11, 2023

Problem

Address #91

Solution

When encountering such byte array represented "Decimal" fields, parse them into raw buffers.

Change summary:

  • Added code to parse "Decimal" type fields represented by byte arrays (fixed length or non-fixed length) into raw buffer values for further client side processing.
  • Added two test cases verifying the added code.
  • Loosen the precision check to allow values greater than 18 for byte array represented "Decimal" fields.

Steps to Verify:

  • Use the library to open a parquet file which contains a "Decimal" field represented by a byte array whose precision is greater than 18.
  • Before the change, library will throw an error saying precision cannot be greater than 18.
  • After the change, library will parse those fields to their raw buffer values and return records normally.

@YECHUNAN YECHUNAN marked this pull request as ready for review August 13, 2023 04:02
Copy link
Member

@wilwade wilwade left a 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" },
Copy link
Member

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);
Copy link
Member

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",
Copy link
Member

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.

Copy link
Collaborator

@shannonwells shannonwells left a 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.

@wilwade wilwade merged commit c07e7e8 into LibertyDSNP:main Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants