Add DELTA_LENGTH_BYTE_ARRAY and DELTA_BYTE_ARRAY encodings

sunchao / parquet-rs

Apache Parquet implementation in Rust

Apache License 2.0

149 stars 20 forks source link

Add DELTA_LENGTH_BYTE_ARRAY and DELTA_BYTE_ARRAY encodings #38

Closed sadikovi closed 6 years ago

sadikovi commented 6 years ago

This PR adds DELTA_LENGTH_BYTE_ARRAY and DELTA_BYTE_ARRAY encodings and fixes some small issues in decodings.

Changes:

Added DELTA_LENGTH_BYTE_ARRAY encoding
Added DELTA_BYTE_ARRAY encoding
Added slice() method for ByteArray
Reordered list of encodings/decodings to be in the same order in get_encoder and get_decoder
Updated DELTA_BYTE_ARRAY decoding get() method, fixed current_idx update, and modified overall method to handle prefix length 0 (IMHO, it is a bit simpler now)
Updated test function and generalized for all delta encodings, added tests for delta byte array decoding (tests delta length byte array decoding implicitly)
Updated generic encoding test to include delta length byte array and delta byte array encoding/decoding

Fixes #33 Fixes #34

sadikovi commented 6 years ago

@sunchao Could you review this PR too? It adds byte array encodings and fixes some issues to make them work. I only tested with unit tests, not with actual files though. Also apologies for changing get() method in DeltaByteArrayDecoder, it actually only needed couple of changes, but I thought that these changes made method simpler and easier to understand what is going on - happy to revert it to the original code!

Would love to know your ideas and suggestions for the change. Thanks for merging previous patch - it makes this patch work!

coveralls commented 6 years ago

Coverage increased (+1.2%) to 92.227% when pulling 520bb9539548d7e17fdefc86e81771f72ca77f87 on sadikovi:delta-byte-array-encoding-2 into 1d8c346903dace15f132d456e1220007fcca05a2 on sunchao:master.

sunchao commented 6 years ago

Thanks @sadikovi ! and sorry for the delay. I'll take a look at this PR soon :)

sadikovi commented 6 years ago

@sunchao all good, no worries:) whenever you have time to review it.

sadikovi commented 6 years ago

@sunchao Would you mind reviewing this PR again? I addressed some of the comments and replied to your inline comments. Would appreciate if you could have a look. Thanks!

sadikovi commented 6 years ago

@sunchao Thank you for the review. I updated the code according to your changes.

Reverted bytes_written, it did not affect anything + there is no need to mix changes in one PR
Added TODO item, it looks like it is applicable for both delta length byte array and delta byte array encodings
Now I clone byte arrays in put method in delta length byte array encoding and only copy data when flush_buffer is called.

Could you review again, when you have time? Thanks. Feel free to comment on any changes, because it helps me to avoid "Java-like" coding, and start thinking on how to avoid extensive data copy.

sadikovi commented 6 years ago

@sunchao Thanks for merging!