GH-35519: [C++][Parquet] Fixing exception handling in parquet FileSerializer #35520

mapleFU · 2023-05-09T16:47:56Z

Rationale for this change

Mentioned in #35519

What changes are included in this PR?

Exception handling in CheckRowsWritten

Are these changes tested?

No, I don't know how to mock it currently

Are there any user-facing changes?

no

Closes: [C++][Parquet] Segfault when exception handling and closing RowGroupSerializer #35519

github-actions · 2023-05-09T16:48:20Z

Closes: [C++][Parquet] Segfault when exception handling and closing RowGroupSerializer #35519

mapleFU · 2023-05-09T16:48:20Z

@pitrou Would you mind take a look? Analyze is in #35519 (comment)

github-actions · 2023-05-09T16:48:22Z

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

pitrou · 2023-05-09T17:03:15Z

I think we should instead prevent RowGroupSerializer::Close from creating an invalid internal state. For example:

  void Close() override {
    if (!closed_) {
      closed_ = true;
      CheckRowsWritten();

      auto column_writers = std::move(columns_writers_);
      for (const auto writer : column_writers) {
        if (writer) {
          total_bytes_written_ += writer->Close();
          total_compressed_bytes_written_ +=
              writer->total_compressed_bytes_written();
        }
      }

      // Ensures all columns have been written
      metadata_->set_num_rows(num_rows_);
      metadata_->Finish(total_bytes_written_, row_group_ordinal_);
    }
  }

mapleFU · 2023-05-09T17:22:59Z

@pitrou Good Idea! Fix as your advice

cpp/src/parquet/file_writer.cc

pitrou · 2023-05-09T17:27:58Z

cpp/src/parquet/file_writer.cc

@@ -380,15 +382,15 @@ class FileSerializer : public ParquetFileWriter::Contents {
  void AddKeyValueMetadata(
      const std::shared_ptr<const KeyValueMetadata>& key_value_metadata) override {
    if (key_value_metadata_ == nullptr) {
-      key_value_metadata_ = std::move(key_value_metadata);
+      key_value_metadata_ = key_value_metadata;


Good catch!

wgtmac · 2023-05-10T07:36:05Z

cpp/src/parquet/file_writer.cc

+      // If ColumnWriter::Close thrown an exception, avoid
+      // have bad states in column_writers_.


Suggested change

// If ColumnWriter::Close thrown an exception, avoid

// have bad states in column_writers_.

// Avoid invalid state if ColumnWriter::Close() throws internally.

If anything wrong happens here, should we let the FileSerializer know that it cannot accept any write any more?

it's already closed, whether success or not, no any write can comes to RowGroupSerializer. I guess it will cause FileSerializer destruct

This is true if the exception happens when calling FileSerializer::Close(). What if this happens when the user directly calls RowGroupWriter::Close() to close the current row group and starts a new one?

void Close() override { if (!closed_) { closed_ = true; CheckRowsWritten(); // ... // Ensures all columns have been written metadata_->set_num_rows(num_rows_); metadata_->Finish(total_bytes_written_, row_group_ordinal_); } }

I guess it will mark closed_ first and throw exception. During stack unwind, the RowGroupSerializer will destruct. And during next time Close is called, it will do nothing

mapleFU · 2023-05-10T13:19:57Z

Comment resolved. I tried to solve some CI problems here: #35527

Seems CI failed frequently these days...

pitrou · 2023-05-10T15:08:21Z

Seems CI failed frequently these days...

Unfortunately :-( Are there issues already open for these failures?

pitrou

Thanks @mapleFU . This looks ok now.

pitrou · 2023-05-10T15:29:18Z

Hmm, the R CI failure may be related as it seems to succeed on git main:
https://github.com/apache/arrow/actions/runs/4937417890/jobs/8826087049

mapleFU · 2023-05-10T15:41:21Z

Yeah, I don't want to rebase this because there are still too much failed tests :-(
Just wait others idea on this patch or merge if you think it's ok

pitrou · 2023-05-10T15:44:00Z

We can't merge until we make sure the R CI failure is unrelated.

pitrou · 2023-05-10T16:34:13Z

Hmm, actually the segfault is already known:
#35391

mapleFU · 2023-05-10T16:35:58Z

😭Hurt by so many failed

mapleFU · 2023-05-11T18:03:11Z

[ RUN      ] TestScan/TestCsvFileFormatScan.ScanBatchSize/0Threaded16b1024r
unknown file: error: SEH exception with code 0xc00000fd thrown in the test body.
[  FAILED  ] TestScan/TestCsvFileFormatScan.ScanBatchSize/0Threaded16b1024r, where GetParam() = Threaded16b1024r (314 ms)

I guess this is unrelated. https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/47021836

mapleFU · 2023-05-11T19:01:09Z

@pitrou Seems R failed is not related. Would you mind take a look?

pitrou · 2023-05-15T15:21:54Z

I'm restarting CI a last time but it looks like this is ready to go.

…ileSerializer (apache#35520) ### Rationale for this change Mentioned in apache#35519 ### What changes are included in this PR? Exception handling in `CheckRowsWritten` ### Are these changes tested? No, I don't know how to mock it currently ### Are there any user-facing changes? no * Closes: apache#35519 Authored-by: mwish <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

ursabot · 2023-05-16T07:23:16Z

Benchmark runs are scheduled for baseline = e2e3a9d and contender = 2d76d9a. 2d76d9a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.26% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.51% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.42% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 2d76d9a5 ec2-t3-xlarge-us-east-2
[Finished] 2d76d9a5 test-mac-arm
[Finished] 2d76d9a5 ursa-i9-9960x
[Finished] 2d76d9a5 ursa-thinkcentre-m75q
[Finished] e2e3a9df ec2-t3-xlarge-us-east-2
[Finished] e2e3a9df test-mac-arm
[Finished] e2e3a9df ursa-i9-9960x
[Finished] e2e3a9df ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

…ileSerializer (apache#35520) ### Rationale for this change Mentioned in apache#35519 ### What changes are included in this PR? Exception handling in `CheckRowsWritten` ### Are these changes tested? No, I don't know how to mock it currently ### Are there any user-facing changes? no * Closes: apache#35519 Authored-by: mwish <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

…ializer (#35520) ### Rationale for this change Mentioned in #35519 ### What changes are included in this PR? Exception handling in `CheckRowsWritten` ### Are these changes tested? No, I don't know how to mock it currently ### Are there any user-facing changes? no * Closes: #35519 Authored-by: mwish <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

Fixes #23606, where memory leaks are introduced by `_chunk_writer->release()` The modification on Arrow is to make sure `RowGroupSerializer::column_writers_` is cleaned up even if exception throws. You may refer apache/arrow#35520 for more details. Signed-off-by: Letian Jiang <[email protected]>

fix npe in parquet

17713ec

mapleFU requested a review from wjones127 as a code owner May 9, 2023 16:47

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

mapleFU added 2 commits May 10, 2023 01:16

address comment

90ec791

Merge branch 'main' into parquet/fixing-npe

76b5fde

pitrou reviewed May 9, 2023

View reviewed changes

cpp/src/parquet/file_writer.cc Outdated Show resolved Hide resolved

pitrou reviewed May 9, 2023

View reviewed changes

cpp/src/parquet/file_writer.cc Show resolved Hide resolved

pitrou reviewed May 9, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 9, 2023

address commit

f43e241

wgtmac reviewed May 10, 2023

View reviewed changes

mapleFU added 2 commits May 10, 2023 17:55

Merge branch 'main' into parquet/fixing-npe

9ea4f59

fix comment

2262cec

pitrou approved these changes May 10, 2023

View reviewed changes

Merge branch 'main' into parquet/fixing-npe

2d7cb27

Merge branch 'main' into parquet/fixing-npe

086b319

mapleFU requested a review from pitrou May 15, 2023 14:11

pitrou merged commit 2d76d9a into apache:main May 15, 2023

mapleFU deleted the parquet/fixing-npe branch May 16, 2023 02:46

letian-jiang mentioned this pull request Jun 20, 2023

[BugFix] Fix arrow parquet exception handling StarRocks/starrocks#25621

Merged

16 tasks

mapleFU mentioned this pull request Oct 7, 2023

[C++][Parquet] Debug Build might fail in DictEncoder dtor checking #38117

Closed

BePPPower mentioned this pull request May 21, 2024

[Fix](Outfile) upgrade apache-arrow version to 13.0.0 apache/doris#35142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-35519: [C++][Parquet] Fixing exception handling in parquet FileSerializer #35520

GH-35519: [C++][Parquet] Fixing exception handling in parquet FileSerializer #35520

mapleFU commented May 9, 2023 •

edited by github-actions bot

Loading

github-actions bot commented May 9, 2023

mapleFU commented May 9, 2023

github-actions bot commented May 9, 2023

pitrou commented May 9, 2023

mapleFU commented May 9, 2023

pitrou May 9, 2023

wgtmac May 10, 2023

wgtmac May 10, 2023

mapleFU May 10, 2023

wgtmac May 10, 2023

mapleFU May 10, 2023

mapleFU commented May 10, 2023

pitrou commented May 10, 2023

pitrou left a comment

pitrou commented May 10, 2023

mapleFU commented May 10, 2023

pitrou commented May 10, 2023

pitrou commented May 10, 2023

mapleFU commented May 10, 2023

mapleFU commented May 11, 2023

mapleFU commented May 11, 2023

pitrou commented May 15, 2023

ursabot commented May 16, 2023

		// If ColumnWriter::Close thrown an exception, avoid
		// have bad states in column_writers_.

	// If ColumnWriter::Close thrown an exception, avoid
	// have bad states in column_writers_.
	// Avoid invalid state if ColumnWriter::Close() throws internally.

GH-35519: [C++][Parquet] Fixing exception handling in parquet FileSerializer #35520

GH-35519: [C++][Parquet] Fixing exception handling in parquet FileSerializer #35520

Conversation

mapleFU commented May 9, 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 May 9, 2023

mapleFU commented May 9, 2023

github-actions bot commented May 9, 2023

pitrou commented May 9, 2023

mapleFU commented May 9, 2023

pitrou May 9, 2023

Choose a reason for hiding this comment

wgtmac May 10, 2023

Choose a reason for hiding this comment

wgtmac May 10, 2023

Choose a reason for hiding this comment

mapleFU May 10, 2023

Choose a reason for hiding this comment

wgtmac May 10, 2023

Choose a reason for hiding this comment

mapleFU May 10, 2023

Choose a reason for hiding this comment

mapleFU commented May 10, 2023

pitrou commented May 10, 2023

pitrou left a comment

Choose a reason for hiding this comment

pitrou commented May 10, 2023

mapleFU commented May 10, 2023

pitrou commented May 10, 2023

pitrou commented May 10, 2023

mapleFU commented May 10, 2023

mapleFU commented May 11, 2023

mapleFU commented May 11, 2023

pitrou commented May 15, 2023

ursabot commented May 16, 2023

mapleFU commented May 9, 2023 •

edited by github-actions bot

Loading