Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

PARQUET-1160: [C++] Implement BYTE_ARRAY-backed Decimal reads #495

Closed

Conversation

thaining
Copy link

This change enables support for DECIMALs backed by BYTE_ARRAYs
on disk. It does this by creating a TransferFunctor routine that
transforms a ByteArrayType to an ::arrow::Decimal128Type.

The routine does this by:

  1. Creating an arrow::BinaryArray from the RecordReader's builder
  2. Allocating a buffer for the arrow::Decimal128Array
  3. Converting the big-endian bytes in each BinaryArray entry to two integers
    representing the high and low bits of each decimal value.

on disk.  It does this by creating a TransferFunctor routine that
transforms a ByteArrayType to an ::arrow::Decimal128Type.

The routine does this by:
1. Creating an arrow::BinaryArray from the RecordReader's builder
2. Allocating a buffer for the arrow::Decimal128Array
3. Converting the big-endian bytes in each BinaryArray entry to two integers
   representing the high and low bits of each decimal value.
Copy link
Member

@xhochy xhochy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Can you add a unit test that verifies the functionality?

@xhochy
Copy link
Member

xhochy commented Sep 5, 2018

@thaining Travis reports some code style issues you could probably get rid of by running make format. Can you take care of that?

@thaining
Copy link
Author

thaining commented Sep 5, 2018

Fixed the indenting and column width changes in reader.cc suggested by 'make format' with clang-format 5.0.2. For arrow-reader-writer-test.cc, the program only suggested reordering of the using statements, which I had not touched.

@thaining
Copy link
Author

thaining commented Sep 5, 2018

@xhochy, a unit test conforming to the others for Spark was also included in the last batch of commits.

@wesm
Copy link
Member

wesm commented Sep 9, 2018

This PR needs to be moved to https://github.com/apache/arrow -- please let us know if you require assistance

@thaining
Copy link
Author

This PR needs to be moved to https://github.com/apache/arrow -- please let us know if you require assistance

Looks like I do require assistance. The change squashes down to a clean commit, but I do not have write access to the repo. Can you advise me on next steps? Thanks.

Ted

@wesm
Copy link
Member

wesm commented Sep 27, 2018

I rolled up your changes as this commit wesm/arrow@4f68fc3. You can open a PR starting from there

@thaining
Copy link
Author

Thanks for the help. I was a little confused, and that gave me the push I needed. I've created PR apache/arrow#2646

@wesm wesm closed this Sep 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants