Skip to content

GH-38432: [C++][Parquet] Try to fix performance regression in the DictByteArrayDecoderImpl #38784

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.

Sign up for GitHub

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

Merged
pitrou merged 11 commits into from
Nov 27, 2023
Merged

GH-38432: [C++][Parquet] Try to fix performance regression in the DictByteArrayDecoderImpl #38784

Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a7633d0
GH-38432: [C++][Parquet] Encoding: Dict Arrow Decoder tiny regression…
mapleFU Nov 20, 2023
f74b4c1
Apply suggestions from code review
mapleFU Nov 20, 2023
7686572
tiny update
mapleFU Nov 21, 2023
5f12876
add comment for ByteArray dict Prepare
mapleFU Nov 21, 2023
51e6fb7
update the allocation length
mapleFU Nov 23, 2023
f3bba12
Merge branch 'main' into dict-decoder-regression-fix
mapleFU Nov 23, 2023
0333b41
[Update] Update maximum logic for Prepare
mapleFU Nov 23, 2023
4f26c98
[Update] Trying to disable "Prepare"
mapleFU Nov 24, 2023
e759a90
Update the impl
mapleFU Nov 24, 2023
60cdb80
resume logic for PrepareNextInput
mapleFU Nov 24, 2023
3dc536c
fix format
mapleFU Nov 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion cpp/src/parquet/encoding.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,6 +1196,10 @@ struct ArrowBinaryHelper<ByteArrayType> {
chunk_space_remaining_(::arrow::kBinaryMemoryLimit -
acc_->builder->value_data_length()) {}

// Prepare will reserve the number of entries remaining in the current chunk.
// If estimated_data_length is provided, it will also Reserve the estimated data length,
// and the caller should remember to call `UnsafeAppend` instead of `Append` to avoid
// double counting the data length.
Status Prepare(std::optional<int64_t> estimated_data_length = {}) {
RETURN_NOT_OK(acc_->builder->Reserve(entries_remaining_));
if (estimated_data_length.has_value()) {
Expand All @@ -1205,6 +1209,9 @@ struct ArrowBinaryHelper<ByteArrayType> {
return Status::OK();
}

@pitrou pitrou Nov 24, 2023

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.

What difference does it make to have a separate method?

@mapleFU mapleFU Nov 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

https://godbolt.org/z/5bf1K55a5

Found it might not optimized with hot path. Prepare is not in hot path, but PrepareNextInput is.


// If estimated_remaining_data_length is provided, it will also reserve the estimated
// data length, and the caller should remember to call `UnsafeAppend` instead of
// `Append` to avoid double counting the data length.
Status PrepareNextInput(int64_t next_value_length,
std::optional<int64_t> estimated_remaining_data_length = {}) {
if (ARROW_PREDICT_FALSE(!CanFit(next_value_length))) {
Expand Down Expand Up @@ -1983,7 +1990,7 @@ class DictByteArrayDecoderImpl : public DictDecoderImpl<ByteArrayType>,
int values_decoded = 0;

ArrowBinaryHelper<ByteArrayType> helper(out, num_values);
RETURN_NOT_OK(helper.Prepare(len_));
RETURN_NOT_OK(helper.Prepare());
Comment thread
mapleFU marked this conversation as resolved.

auto dict_values = reinterpret_cast<const ByteArray*>(dictionary_->data());

Comment thread
mapleFU marked this conversation as resolved.
Expand Down