GH-39336: [C++][Parquet] Minor: Style enhancement for parquet::FileMetaData #39337

mapleFU · 2023-12-21T13:10:55Z

Rationale for this change

Enhance the style of parquet::FileMetaData::Subset.

Are these changes tested?

Already tested

Are there any user-facing changes?

no

Closes: [C++][Parquet] Minor: Style enhancement for parquet::FileMetaData #39336

mapleFU · 2023-12-21T13:11:09Z

cc @wgtmac @pitrou

github-actions · 2023-12-21T13:11:21Z

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

cpp/src/parquet/metadata.h

pitrou

LGTM, but see docstring suggestions

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

mapleFU · 2023-12-21T15:23:41Z

comment fixed now

mapleFU · 2023-12-22T02:47:27Z

cpp/src/parquet/metadata.cc

+    // Discard row groups that are not in the subset
+    output_metadata->num_rows = 0;
+    output_metadata->row_groups.clear();


Note:

Here will introduce a more round for copying metadata_->row_groups.

Should I:

bool isset_rowgroup = metadata->isset.row_groups; metadata->isset.row_groups = false; auto row_groups = std::move(metadata->row_groups); // Copying thrift // ... // Assign row_groups back metadata->row_groups = std::move(row_groups); metadata->isset.row_groups = ... // .. Handle RowGroups

? cc @wgtmac @pitrou

This looks reasonable to me.

cpp/src/parquet/metadata.cc

mapleFU · 2024-01-03T02:50:40Z

cpp/src/parquet/metadata.cc

@@ -1839,7 +1843,7 @@ class FileMetaDataBuilder::FileMetaDataBuilderImpl {
  std::unique_ptr<FileMetaData> Finish(
      const std::shared_ptr<const KeyValueMetadata>& key_value_metadata) {
    int64_t total_rows = 0;
-    for (auto row_group : row_groups_) {
+    for (const auto& row_group : row_groups_) {


This is another problem I found during reading the code

mapleFU · 2024-01-08T16:01:36Z

@pitrou @wgtmac I've updated the logic for copying row-groups, would you mind take a look again?

wgtmac · 2024-01-09T04:51:31Z

cpp/src/parquet/metadata.cc

+    {
+      // GH-39336: Copy the metadata, but only the row groups we need.
+      auto temp_row_groups = std::move(this->metadata_->row_groups);
+      try {


Why do you add try-catch block?

I think make_unique and copying might throw exception( like std::bad_alloc), and even when ex is thrown, we'd better keep this not changed (otherwise it would be hard to debugging?)

I'm not sure if it worths this kind of complexity to handle the exception just in order to avoid the copy.

pitrou · 2024-01-10T17:06:24Z

cpp/src/parquet/metadata.cc

-    metadata->schema = metadata_->schema;
+    {
+      // GH-39336: Copy the metadata, but only the row groups we need.
+      auto temp_row_groups = std::move(this->metadata_->row_groups);


What is the point of doing this? Overall, are these changes actually bringing a tangible benefit?

The origin code don't copying the previous row-group. If we have a large file with many row-groups (it's possible), the more around of copy waste a lot of cpu time. So I do this as an optimization.

My problem is that you're temporarily mutating this in a method that's not supposed to mutate it. This is brittle (look at that try/except) and also thread-unsafe.

You're right, the thread-safety is a problem here. Maybe keeping the current code is ok

mapleFU · 2024-01-17T11:14:35Z

@wgtmac @pitrou I remove the Subset change in pr and issue. This pr just change the style, and reduce some copy using reference or std::move

conbench-apache-arrow · 2024-01-17T22:45:50Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit f15a700.

There were 3 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2024-01-17 19:40:59Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-20, scale_factor=1
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-16, scale_factor=1
and 1 more (see the report linked below)

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

…FileMetaData (apache#39337) ### Rationale for this change Enhance the style of `parquet::FileMetaData::Subset`. ### Are these changes tested? Already tested ### Are there any user-facing changes? no * Closes: apache#39336 Lead-authored-by: mwish <maplewish117@gmail.com> Co-authored-by: mwish <1506118561@qq.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr> Signed-off-by: mwish <maplewish117@gmail.com>

Minor: style enhancment for parquet FileMetadata Subset

ac0c569

mapleFU requested a review from wgtmac as a code owner December 21, 2023 13:10

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

pitrou reviewed Dec 21, 2023

View reviewed changes

cpp/src/parquet/metadata.h Outdated Show resolved Hide resolved

pitrou reviewed Dec 21, 2023

View reviewed changes

cpp/src/parquet/metadata.h Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Dec 21, 2023

pitrou requested changes Dec 21, 2023

View reviewed changes

Apply suggestions from code review

b15990d

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

pitrou approved these changes Dec 21, 2023

View reviewed changes

wgtmac approved these changes Dec 22, 2023

View reviewed changes

mapleFU commented Dec 22, 2023

View reviewed changes

mapleFU added 3 commits December 22, 2023 11:28

enhance the style for metadata AppendRowGroup

5b4ccc0

Merge branch 'main' into tiny-metadata-style-enhancement

acef18b

Update: avoid copying row-group

acd6ab2

mapleFU commented Jan 3, 2024

View reviewed changes

wgtmac reviewed Jan 9, 2024

View reviewed changes

mapleFU requested review from pitrou and wgtmac January 10, 2024 07:37

pitrou reviewed Jan 10, 2024

View reviewed changes

mapleFU closed this Jan 17, 2024

mapleFU changed the title ~~GH-39336: [C++][Parquet] Minor: Style enhancement for parquet::FileMetaData::Subset~~ GH-39336: [C++][Parquet] Minor: Style enhancement for parquet::FileMetaData Jan 17, 2024

mapleFU reopened this Jan 17, 2024

Merge branch 'main' into tiny-metadata-style-enhancement

4ee53ba

Revert Subset changes

1a2be3d

pitrou approved these changes Jan 17, 2024

View reviewed changes

mapleFU merged commit f15a700 into apache:main Jan 17, 2024
34 checks passed

mapleFU removed the awaiting committer review Awaiting committer review label Jan 17, 2024

mapleFU deleted the tiny-metadata-style-enhancement branch January 17, 2024 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-39336: [C++][Parquet] Minor: Style enhancement for parquet::FileMetaData #39337

GH-39336: [C++][Parquet] Minor: Style enhancement for parquet::FileMetaData #39337

mapleFU commented Dec 21, 2023 •

edited by pitrou

Loading

mapleFU commented Dec 21, 2023

github-actions bot commented Dec 21, 2023

pitrou left a comment

mapleFU commented Dec 21, 2023

mapleFU Dec 22, 2023

wgtmac Dec 25, 2023

mapleFU Jan 3, 2024

mapleFU commented Jan 8, 2024

wgtmac Jan 9, 2024

mapleFU Jan 9, 2024

wgtmac Jan 10, 2024

pitrou Jan 10, 2024

mapleFU Jan 17, 2024

pitrou Jan 17, 2024

mapleFU Jan 17, 2024 •

edited

Loading

mapleFU commented Jan 17, 2024

conbench-apache-arrow bot commented Jan 17, 2024

GH-39336: [C++][Parquet] Minor: Style enhancement for parquet::FileMetaData #39337

GH-39336: [C++][Parquet] Minor: Style enhancement for parquet::FileMetaData #39337

Conversation

mapleFU commented Dec 21, 2023 • edited by pitrou Loading

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

mapleFU commented Dec 21, 2023

github-actions bot commented Dec 21, 2023

pitrou left a comment

Choose a reason for hiding this comment

mapleFU commented Dec 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

mapleFU commented Jan 17, 2024

conbench-apache-arrow bot commented Jan 17, 2024

mapleFU commented Dec 21, 2023 •

edited by pitrou

Loading

mapleFU Jan 17, 2024 •

edited

Loading