Skip to content

PARQUET-490: [C++][Parquet] Basic support for reading DELTA_BINARY_PACKED data - #10627

Closed
shanhuuang wants to merge 9 commits into
apache:masterfrom
shanhuuang:ARROW-13206
Closed

PARQUET-490: [C++][Parquet] Basic support for reading DELTA_BINARY_PACKED data#10627
shanhuuang wants to merge 9 commits into
apache:masterfrom
shanhuuang:ARROW-13206

Conversation

@shanhuuang

@shanhuuangshanhuuang commented Jun 30, 2021•
edited by pitrou
Loading

Copy link
Copy Markdown
Contributor

Only works for int32/int64 when delta_bit_width_<=32.

  1. Update the implement of DeltaBitPackDecoder
  2. Revise the code of putting/getting ZigZag int and the unit test
  3. Add the implement of putting/getting ZigZag int64 and unit test

@github-actions

github-actionsBot commented Jun 30, 2021

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-13206

@shanhuuang
shanhuuangforce-pushed the ARROW-13206 branch 3 times, most recently from c0b68b2 to e01f698CompareJuly 1, 2021 02:28
pincheng.hs
ARROW-13206: [C++][Parquet] Basic support for encoding DELTA_BINARY_P…
cf4a51f
…ACKED. Only works for int32/int64 when delta_bit_width_<=32
@shanhuuang
shanhuuangforce-pushed the ARROW-13206 branch 6 times, most recently from 0512004 to 058bc0eCompareJuly 5, 2021 07:52
@jorisvandenbosschejorisvandenbossche changed the title ARROW-13206: [C++][Parquet] Basic support for encoding DELTA_BINARY_P…ARROW-13206: [C++][Parquet] Basic support for encoding DELTA_BINARY_PACKEDJul 5, 2021
@shanhuuang
shanhuuangforce-pushed the ARROW-13206 branch 2 times, most recently from 2b3b1d1 to 69fbac2CompareJuly 6, 2021 13:34
emkornfield
emkornfield reviewed Jul 6, 2021
View reviewed changes
Comment threadcpp/src/arrow/util/bit_stream_utils.h Outdated
emkornfield
emkornfield reviewed Jul 6, 2021
View reviewed changes
Comment threadcpp/src/arrow/util/bit_stream_utils.h
emkornfield
emkornfield reviewed Jul 6, 2021
View reviewed changes
Comment threadcpp/src/arrow/util/bit_stream_utils.h Outdated
emkornfield
emkornfield reviewed Jul 6, 2021
View reviewed changes
Comment threadcpp/src/arrow/util/bpacking.cc
emkornfield
emkornfield reviewed Jul 6, 2021
View reviewed changes
Comment threadcpp/src/arrow/util/bpacking64_default.h
emkornfield
emkornfield reviewed Jul 6, 2021
View reviewed changes
Comment threadcpp/src/parquet/encoding.cc
explicit DeltaBitPackDecoder(const ColumnDescriptor* descr,
MemoryPool* pool = ::arrow::default_memory_pool())
: DecoderImpl(descr, Encoding::DELTA_BINARY_PACKED), pool_(pool) {
if (DType::type_num != Type::INT32 && DType::type_num != Type::INT64) {

@emkornfieldemkornfieldJul 6, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why remove this check?

@shanhuuangshanhuuangJul 7, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I thought that DeltaBitPackDecoder object would only be created in function parquet::MakeDecoder, so no more check is needed here. It's ok to restore this code.

emkornfield
emkornfield reviewed Jul 6, 2021
View reviewed changes
Comment threadcpp/src/parquet/reader_test.cc Outdated
@emkornfieldemkornfield changed the title ARROW-13206: [C++][Parquet] Basic support for encoding DELTA_BINARY_PACKEDPARQUET-490: [C++][Parquet] Basic support for encoding DELTA_BINARY_PACKEDJul 6, 2021
@github-actions

github-actionsBot commented Jul 6, 2021

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/PARQUET-490

@emkornfield

emkornfield commented Jul 6, 2021

Copy link
Copy Markdown
Contributor

Is it possible for delta_bit_width_<=32? if so it seems like we should probably cover that case while updating?

@emkornfield

emkornfield commented Jul 6, 2021

Copy link
Copy Markdown
Contributor

CC @zeroshade note bugs fixed in ZigZag/VLQ int, not sure if these got propagated to Go.

@zeroshade

zeroshade commented Jul 6, 2021

Copy link
Copy Markdown
Member

@emkornfield I added the tests that were added here to the Go side, including the comparison of the actual bytes generated and the Golang implementation passes all the tests as currently written without any modifications needed. it appears that the Golang version doesn't have this bug (I remember coming across a bug in the ZigZag/VLQ int stuff a while back where I had to manage the signs and such to get the right output so I likely fixed it without realizing).

@shanhuuang

shanhuuang commented Aug 3, 2021

Copy link
Copy Markdown
ContributorAuthor

sorry I have had less time then I would have liked recently for Arrow reviews, will try to get to this soon.

OK, Thanks a lot :)

emkornfield
emkornfield reviewed Aug 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/util/bit_stream_utils.h
emkornfield
emkornfield reviewed Aug 17, 2021
View reviewed changes
Comment threadcpp/src/parquet/encoding.cc Outdated
@emkornfield

emkornfield commented Aug 17, 2021

Copy link
Copy Markdown
Contributor

Could we create a separate PR for the test files directly to the test file repo?

@emkornfield

emkornfield commented Aug 17, 2021

Copy link
Copy Markdown
Contributor

I might be missing it but is there a test demonstrating current behavior for bit_widths >= 32?

@emkornfield

emkornfield commented Aug 17, 2021

Copy link
Copy Markdown
Contributor

Mostly looks OK, I'm going to try to do a second more careful pass this week. Also CC @pitrou in case he wants to take a look.

@emkornfield

emkornfield commented Aug 17, 2021

Copy link
Copy Markdown
Contributor

Thank you for your patience.

pitrou
pitrou requested changes Aug 17, 2021
View reviewed changes

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for this PR. Are you planning to support encoding in another PR?

Comment threadcpp/src/arrow/util/bit_stream_utils.h
Comment threadcpp/src/arrow/util/bit_util_test.cc Outdated
Comment threadcpp/src/arrow/util/bit_util_test.cc Outdated
Comment threadcpp/src/arrow/util/bpacking64_codegen.py
print(" w{0} = arrow::BitUtil::FromLittleEndian(w{0});".format(k))
print(" in += 8;".format(k))
else:
print(" uint64_t w{0} = util::SafeLoadAs<uint32_t>(in);".format(k))

@pitroupitrouAug 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I find this implementation weird. Why does it not unpack 64 entries at a time? I'm afraid this may make writing the SIMD implementations more difficult.

@pitroupitrouAug 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, is this because DELTA_BINARY_PACKED outputs values in multiples of 32? Slightly annoying :-/

@shanhuuangshanhuuangAug 18, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes. The number of values(including padding) in a miniblock is always a multiple of 32.

Comment threadcpp/src/parquet/reader_test.cc Outdated
Comment threadcpp/src/parquet/arrow/arrow_reader_writer_test.cc
Comment threadcpp/src/parquet/encoding.cc Outdated
Comment threadcpp/src/parquet/encoding.cc Outdated
Comment threadcpp/src/parquet/encoding.cc Outdated
values_current_mini_block_ = values_per_mini_block_;
} else {
// mini_block_idx_ > mini_blocks_per_block_ only if last_value_ is stored in
// header

@pitroupitrouAug 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't understand this comment. last_value_ is always stored in header, right?

Comment threadcpp/src/parquet/encoding.cc Outdated
@pitroupitrou changed the title PARQUET-490: [C++][Parquet] Basic support for encoding DELTA_BINARY_PACKEDPARQUET-490: [C++][Parquet] Basic support for reading DELTA_BINARY_PACKED dataAug 17, 2021
@pitrou

pitrou commented Aug 17, 2021

Copy link
Copy Markdown
Member

Only works for int32/int64 when delta_bit_width_<=32.

What do you mean? Are delta bit widths > 32 not supported? I don't see check in the code.

shanhuuang
shanhuuang commented Aug 19, 2021
View reviewed changes

@shanhuuangshanhuuang left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Only works for int32/int64 when delta_bit_width_<=32.

What do you mean? Are delta bit widths > 32 not supported? I don't see check in the code.

Sorry, this is the commit message when the code was submitted in the first time. delta bit widths > 32 have also been supported now in this PR.

Comment threadcpp/src/arrow/util/bit_stream_utils.h
Comment threadcpp/src/arrow/util/bit_util_test.cc Outdated
Comment threadcpp/src/arrow/util/bpacking64_codegen.py
print(" w{0} = arrow::BitUtil::FromLittleEndian(w{0});".format(k))
print(" in += 8;".format(k))
else:
print(" uint64_t w{0} = util::SafeLoadAs<uint32_t>(in);".format(k))

@shanhuuangshanhuuangAug 18, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes. The number of values(including padding) in a miniblock is always a multiple of 32.

Comment threadcpp/src/parquet/encoding.cc Outdated
Comment threadcpp/src/parquet/encoding.cc Outdated
@shanhuuang

shanhuuang commented Aug 19, 2021

Copy link
Copy Markdown
ContributorAuthor

Thank you for this PR. Are you planning to support encoding in another PR?

Yes. I'm also working on PARQUET-491 and PARQUET-492 which depend on code in this PR. And I would like to create a new PR after this one is merged.

@shanhuuang
Modify the code and test case according to reviews.
4251e2f
A boolean member named "block_initialized_" is added in class DeltaLengthByteArrayDecoder to indicate whether function "InitBlock" has been called once.
emkornfield
emkornfield reviewed Aug 19, 2021
View reviewed changes
Comment threadcpp/src/parquet/CMakeLists.txt
add_definitions(-DARROW_WITH_ZSTD)
endif()

if(ARROW_CSV)

@emkornfieldemkornfieldAug 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this seems out of place?

@shanhuuangshanhuuangAug 19, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Do you means that I should add the code in function ADD_PARQUET_TEST? Something like:

 if(REL_TEST_NAME STREQUAL "arrow-test" AND ARROW_CSV)
add_definitions(-DARROW_CSV)
endif()

This one also works :)

@pitroupitrouAug 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems ok to me currently.

pitrou
pitrou requested changes Aug 19, 2021
View reviewed changes

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is starting to look quite good. Can you update the parquet-testing submodule now that apache/parquet-testing#19 has been merged?

Also, here are a couple more comments.

Comment threadcpp/src/arrow/util/bit_stream_utils.h Outdated
i += num_unpacked;
byte_offset += num_unpacked * num_bits / 8;
} else if (sizeof(T) == 8 && num_bits > 32) {
// Use unpack64 only if num_bits is larger then 32

@pitroupitrouAug 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I hadn't noticed previously, but why? Is it just a performance issue?

@shanhuuangshanhuuangAug 19, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, I think it is just a performance issue. If num_bits is no larger than 32, I guess that using unpack32 will achieve better performance with function such as unpack32_avx2/unpack32_avx512.
The result of using unpack32 or unpack64 are the same if num_bits <= 32

Comment threadcpp/src/arrow/util/bit_stream_utils.h
Comment threadcpp/src/parquet/CMakeLists.txt
add_definitions(-DARROW_WITH_ZSTD)
endif()

if(ARROW_CSV)

@pitroupitrouAug 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems ok to me currently.

Comment threadcpp/src/arrow/util/bit_util_test.cc Outdated
Comment threadcpp/src/parquet/arrow/arrow_reader_writer_test.cc
@pitrou

pitrou commented Aug 19, 2021

Copy link
Copy Markdown
Member

@shanhuuang Do you have a user id on the Apache JIRA tracker, so that I can assign the issue to you?

@shanhuuang

shanhuuang commented Aug 20, 2021

Copy link
Copy Markdown
ContributorAuthor

@shanhuuang Do you have a user id on the Apache JIRA tracker, so that I can assign the issue to you?

Yes, my username on JIRA is "shanhuang". Thanks! :)

@shanhuuang
shanhuuang deleted the ARROW-13206 branch August 20, 2021 02:02
@mapleFUmapleFU mentioned this pull request Dec 30, 2022
Merged
pitrou added a commit that referenced this pull request Jan 4, 2023
@mapleFU@pitrou@rok
GH-15052: [C++][Parquet] Fix DELTA_BINARY_PACKED decoder when reading…
53d73f8
… only one value (#15124)
This patch trying to fix#15052 . The problem is mentioned here: #15052 (comment)
When read 1 value, DeltaBitPackDecoder will not call `InitBlock`, causing it always read `last_value_`.
Seems the problem is introduced in #10627 and amol-@d982bed
I will add some test tonight
* Closes: #15052
Lead-authored-by: mwish <maplewish117@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Co-authored-by: mwish <1506118561@qq.com>
Co-authored-by: Rok Mihevc <rok@mihevc.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
EpsilonPrime pushed a commit to EpsilonPrime/arrow that referenced this pull request Jan 5, 2023
@mapleFU@pitrou
@rok@EpsilonPrime
apacheGH-15052: [C++][Parquet] Fix DELTA_BINARY_PACKED decoder when r…
fbc65f3
…eading only one value (apache#15124)
This patch trying to fixapache#15052 . The problem is mentioned here: apache#15052 (comment)
When read 1 value, DeltaBitPackDecoder will not call `InitBlock`, causing it always read `last_value_`.
Seems the problem is introduced in apache#10627 and amol-@d982bed
I will add some test tonight
* Closes: apache#15052
Lead-authored-by: mwish <maplewish117@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Co-authored-by: mwish <1506118561@qq.com>
Co-authored-by: Rok Mihevc <rok@mihevc.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@asfimportasfimport mentioned this pull request Aug 27, 2022
Closed
@asfimportasfimport mentioned this pull request Sep 15, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emkornfieldemkornfieldemkornfield left review comments
@pitroupitroupitrou approved these changes

Assignees

No one assigned

Labels

Component: C++Component: Parquet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shanhuuang@emkornfield@zeroshade@pitrou