GH-38432: [C++][Parquet] Trying to use original logic of BinaryBuilder #38437

mapleFU · 2023-10-24T14:02:42Z

Rationale for this change

Do some changes mentioned in #38432

What changes are included in this PR?

for std::optional argument, it might be used in loop, so separate it
Remove some Prepare.

Are these changes tested?

Already has tests

Are there any user-facing changes?

no

Closes: [C++] Parquet reading performance regressions #38432

github-actions · 2023-10-24T14:03:11Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

raulcd · 2023-10-24T14:05:27Z

@ursabot please benchmark

ursabot · 2023-10-24T14:05:35Z

Benchmark runs are scheduled for commit 2ff9ab4. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

mapleFU · 2023-10-24T14:20:31Z

@ursabot please benchmark

ursabot · 2023-10-24T14:20:37Z

Benchmark runs are scheduled for commit eba5044. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

conbench-apache-arrow · 2023-10-24T19:32:33Z

Thanks for your patience. Conbench analyzed the 0 benchmarking runs that have been run so far on PR commit 2ff9ab4.

None of the specified runs were found on the Conbench server.

The full Conbench report has more details.

conbench-apache-arrow · 2023-10-24T22:27:06Z

Thanks for your patience. Conbench analyzed the 6 benchmarking runs that have been run so far on PR commit eba5044.

There were 4 benchmark results indicating a performance regression:

Pull Request Run on arm64-t4g-linux-compute at 2023-10-24 15:55:54Z
- ReverseSetBitRunReader (C++) with params=99, source=cpp-micro, suite=arrow-bit-util-benchmark
- MinMaxKernelDouble (C++) with params=1048576/1, source=cpp-micro, suite=arrow-acero-aggregate-benchmark
and 2 more (see the report linked below)

The full Conbench report has more details.

jorisvandenbossche · 2023-10-27T07:57:02Z

Looking at the benchmark run, there is a clear improvement in the affected file-read benchmarks. Asking for the comparison of the benchmarked commit with the PR base: https://conbench.ursa.dev/compare/runs/b9dce8c2b76f4c278256da7797cb932d...a810bbfce49c4c238686b2097866fc39/, and sorting by positive z score:

So this seems to solve the issue that is discussed in #38432

mapleFU · 2023-10-27T08:05:15Z

I didn't find some stable improvement from local benchmark, profile and others

But maybe we can first move forward.

mapleFU · 2023-10-27T08:09:45Z

@jorisvandenbossche @pitrou

I've revert some BinaryHelper change. I don't know the reason because I don't have the regression workload. Maybe you can take a look

github-actions · 2023-10-27T08:11:53Z

⚠️ GitHub issue #38432 has been automatically assigned in GitHub to PR creator.

mapleFU · 2023-11-08T15:06:58Z

cpp/src/parquet/encoding.cc

-                          std::optional<int64_t> estimated_remaining_data_length = {}) {
+  Status PrepareNextInput(int64_t next_value_length) {
+    if (ARROW_PREDICT_FALSE(!CanFit(next_value_length))) {
+      // This element would exceed the capacity of a chunk


This part doesn't call RETURN_NOT_OK(acc_->builder->Reserve(entries_remaining_));, this keeps some logic same as before https://github.com/apache/arrow/pull/14341/files , it leave Prepare() to allocate the data and don't push when chunk is switched. But I don't know if this is right

mapleFU · 2023-11-08T15:07:33Z

cpp/src/parquet/encoding.cc

+    return Status::OK();
+  }
+
+  Status PrepareNextInputWithEstimatedLength(int64_t next_value_length,


Split into two functions to make it fast and clear

mapleFU · 2023-11-08T15:09:53Z

cpp/src/parquet/encoding.cc

@@ -1915,7 +1923,6 @@ class DictByteArrayDecoderImpl : public DictDecoderImpl<ByteArrayType>,
    int32_t indices[kBufferSize];

    ArrowBinaryHelper<ByteArrayType> helper(out, num_values);
-    RETURN_NOT_OK(helper.Prepare());


Though I think this Prepare is ok, original code doesn't have this Prepare, perhaps it would make entries large?

mapleFU · 2023-11-08T15:10:15Z

cpp/src/parquet/encoding.cc

@@ -1983,7 +1990,6 @@ class DictByteArrayDecoderImpl : public DictDecoderImpl<ByteArrayType>,
    int values_decoded = 0;

    ArrowBinaryHelper<ByteArrayType> helper(out, num_values);
-    RETURN_NOT_OK(helper.Prepare(len_));


pitrou · 2023-11-08T15:48:25Z

Can you please explain clearly why you're doing these changes? It's not obvious at all that they are correct.

mapleFU · 2023-11-08T15:51:08Z

That's because:

[C++] Parquet reading performance regressions #38432 This issue mentioned a performance regression when using 14.0 and Find that DELTA_BYTE_ARRAY causing build arrow slower. After my change the regression disappear but I don't know why =_=
I think Reading parquet file behavior change from 13.0.0 to 14.0.0 #38577 also raise from same reason, but I don't know if is ok to fix it.

mapleFU · 2023-11-19T07:44:42Z

I've created a simpler patch for this.

github-actions bot added Component: Parquet Component: C++ awaiting review Awaiting review labels Oct 24, 2023

mapleFU mentioned this pull request Oct 24, 2023

[C++] Parquet reading performance regressions #38432

Closed

mapleFU changed the title ~~[C++][Parquet] GH-38432: [DNM] Trying to use original logic of BinaryBuilder~~ MINOR: [C++][Parquet] Trying to use original logic of BinaryBuilder Oct 24, 2023

Prepare apacheGH-38432

eba5044

mapleFU force-pushed the test-binary-helper branch from 2ff9ab4 to eba5044 Compare October 24, 2023 14:08

mapleFU added 2 commits October 27, 2023 16:06

Merge branch 'main' into test-binary-helper

8aa6fa3

cleanup

fec597e

mapleFU marked this pull request as ready for review October 27, 2023 08:09

mapleFU requested a review from wgtmac as a code owner October 27, 2023 08:09

mapleFU changed the title ~~MINOR: [C++][Parquet] Trying to use original logic of BinaryBuilder~~ GH-38432: [C++][Parquet] Trying to use original logic of BinaryBuilder Oct 27, 2023

This was referenced Nov 8, 2023

Reading parquet file behavior change from 13.0.0 to 14.0.0 #38577

Closed

GH-38591: [Parquet][C++] Remove redundant open calls in ParquetFileFormat::GetReaderAsync #38621

Merged

Merge branch 'main' into test-binary-helper

0ef56f2

mapleFU commented Nov 8, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 8, 2023

jorisvandenbossche mentioned this pull request Nov 8, 2023

GH-32863: [C++][Parquet] Add DELTA_BYTE_ARRAY encoder to Parquet writer #14341

Merged

mapleFU closed this Nov 19, 2023

mapleFU deleted the test-binary-helper branch November 19, 2023 07:44

jorisvandenbossche mentioned this pull request Nov 20, 2023

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38432: [C++][Parquet] Trying to use original logic of BinaryBuilder #38437

GH-38432: [C++][Parquet] Trying to use original logic of BinaryBuilder #38437

mapleFU commented Oct 24, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Oct 24, 2023

raulcd commented Oct 24, 2023

ursabot commented Oct 24, 2023

mapleFU commented Oct 24, 2023

ursabot commented Oct 24, 2023

conbench-apache-arrow bot commented Oct 24, 2023

conbench-apache-arrow bot commented Oct 24, 2023

jorisvandenbossche commented Oct 27, 2023

mapleFU commented Oct 27, 2023

mapleFU commented Oct 27, 2023

github-actions bot commented Oct 27, 2023

mapleFU Nov 8, 2023

mapleFU Nov 8, 2023

mapleFU Nov 8, 2023

mapleFU Nov 8, 2023

pitrou commented Nov 8, 2023

mapleFU commented Nov 8, 2023

mapleFU commented Nov 19, 2023

GH-38432: [C++][Parquet] Trying to use original logic of BinaryBuilder #38437

GH-38432: [C++][Parquet] Trying to use original logic of BinaryBuilder #38437

Conversation

mapleFU commented Oct 24, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Oct 24, 2023

raulcd commented Oct 24, 2023

ursabot commented Oct 24, 2023

mapleFU commented Oct 24, 2023

ursabot commented Oct 24, 2023

conbench-apache-arrow bot commented Oct 24, 2023

conbench-apache-arrow bot commented Oct 24, 2023

jorisvandenbossche commented Oct 27, 2023

mapleFU commented Oct 27, 2023

mapleFU commented Oct 27, 2023

github-actions bot commented Oct 27, 2023

mapleFU Nov 8, 2023

Choose a reason for hiding this comment

mapleFU Nov 8, 2023

Choose a reason for hiding this comment

mapleFU Nov 8, 2023

Choose a reason for hiding this comment

mapleFU Nov 8, 2023

Choose a reason for hiding this comment

pitrou commented Nov 8, 2023

mapleFU commented Nov 8, 2023

mapleFU commented Nov 19, 2023

mapleFU commented Oct 24, 2023 •

edited by github-actions bot

Loading