Read Parquet Page Header with ThriftStreamingTransport to Fix the Incorrect Header Length #4108

liushengxuan · 2023-02-22T05:41:28Z

The size of Parquet Page Header remains unknown until the Header is parsed because it is in Thrift format. Without this PR, the size is estimated by sizeof(PageHeader) but the actual Parquet Page Header might be larger. It can be reproduced by using the given largePageHeader. This PR takes the advantage of ThriftStreamingTransport with the input of inputStream_ to avoid estimating the Parquet Page Length. In this case, the incorrect Parquet Page Header length can be resolved.

Also, this approach avoids unnecessary deep copy when reading Parquet Page Headers. Without this PR, deep copying might happen when preparing the input data from in-consecutive chunks for ThriftBufferedTransport.

This PR also introduced the related Parquet page header unit tests on small, large, and corrupted Parquet Page Headers.

netlify · 2023-02-22T05:41:34Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`101a05f`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6406747f95d9eb0008675c35

liushengxuan · 2023-02-22T07:51:37Z

@yingsu00 @Yuhta

velox/dwio/parquet/reader/PageReader.cpp

velox/dwio/parquet/reader/PageReader.h

velox/dwio/parquet/reader/PageReader.cpp

velox/dwio/parquet/reader/PageReader.h

velox/dwio/parquet/reader/PageReader.cpp

…orrect Header Length The size of Parquet Page Header remains unknown until the Header is parsed because it is in Thrift format. Without this PR, the size is estimated by sizeof(PageHeader) but the actual Parquet Page Header might be larger. It can be reproduced by using the given largePageHeader. This PR takes the advantage of ThriftStreamingTransport with the input of inputStream_ to avoid estimating the Parquet Page Length. In this case, the incorrect Parquet Page Header length can be resolved. Also, this approach avoids unnecessary deep copy when reading Parquet Page Headers. Without this PR, deep copying might happen when preparing the input data from in-consecutive chunks for ThriftBufferedTransport. This PR also introduced the related Parquet page header unit tests on small, large, and corrupted Parquet Page Headers.

facebook-github-bot · 2023-03-06T23:27:04Z

@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

…to Fix the Incorrect Header Length facebookincubator#4108

facebook-github-bot · 2023-03-07T19:24:59Z

@Yuhta merged this pull request in 94905a0.

…g data (#145) * Port a patch: Refactor Thrift Transport for Parquet Metadata Access facebookincubator#4160 * Port a patch: Read Parquet Page Header with ThriftStreamingTransport to Fix the Incorrect Header Length facebookincubator#4108

…g data (oap-project#145) * Port a patch: Refactor Thrift Transport for Parquet Metadata Access facebookincubator#4160 * Port a patch: Read Parquet Page Header with ThriftStreamingTransport to Fix the Incorrect Header Length facebookincubator#4108

PHILO-HE · 2023-03-21T08:29:53Z

@liushengxuan, thanks for your work!
Recently, we have verified the patch, no functionality issue. But we observed there is some perf. regression with this patch applied. For TPCH q1, there is around 30% perf. drop. Is there any possible perf. optimization? Thanks!

Yuhta · 2023-03-21T14:18:08Z

@liushengxuan My guess is that it was able to inline the ThriftBufferedTransport::read in old code on hot path, and in new code this is not possible. Do you have time to take a look?

Yuhta · 2023-03-21T14:20:15Z

velox/dwio/parquet/reader/PageReader.cpp

  if (bufferEnd_ == bufferStart_) {
    const void* buffer;
    int32_t size;
    inputStream_->Next(&buffer, &size);
    bufferStart_ = reinterpret_cast<const char*>(buffer);
    bufferEnd_ = bufferStart_ + size;
  }
-  if (bufferEnd_ - bufferStart_ >= sizeof(PageHeader)) {


We probably should keep this branch for hot path

Yuhta · 2023-03-21T14:26:35Z

velox/dwio/parquet/reader/ParquetReader.cpp

-  auto thriftProtocol =
-      std::make_unique<apache::thrift::protocol::TCompactProtocolT<
-          thrift::ThriftBufferedTransport>>(thriftTransport);
+  std::shared_ptr<thrift::ThriftTransport> thriftTransport =


Also undo this should probably could make it a little faster

Yuhta · 2023-03-21T14:30:37Z

velox/dwio/parquet/reader/PageReader.cpp

-        thrift::ThriftBufferedTransport>>(transport);
-  } else {
-    dwio::common::readBytes(
-        std::min<int64_t>(remainingSize, sizeof(PageHeader)),


We probably also need some larger initial read when reading from SeekableInputStream

Yuhta · 2023-03-22T19:00:12Z

cc: @oerling

Yuhta · 2023-03-22T19:02:15Z

Hi @PHILO-HE, @oerling did run TPCH Q1 on our side and we do not see any perf regression. Can you double check and elaborate a little more about the regression you observed?

oerling · 2023-03-22T19:09:25Z

The read of ThriftStreamingTransport occurs at 0.09% in the linux perf profile for running Velox_tpch_benchmark Q1. It can have no bearing on performance. The function is almost always called with a length of 1. So making a special case for that would make it faster. But it is already way below measurement threshold. A 30% drop for Q1 is woryth noting but this is not a valid reason for that. What is the linux perf profile for running this? Thanks Orri From: Jimmy Lu ***@***.***> Sent: Wednesday, March 22, 2023 12:00 PM To: facebookincubator/velox ***@***.***> Cc: oerling ***@***.***>; Mention ***@***.***> Subject: Re: [facebookincubator/velox] Read Parquet Page Header with ThriftStreamingTransport to Fix the Incorrect Header Length (PR #4108) cc: @oerling <https://github.com/oerling> — Reply to this email directly, view it on GitHub <#4108 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKPPPT2PCF6JGZXRQPQFMDTW5ND4RANCNFSM6AAAAAAVD4FCLA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

liushengxuan · 2023-03-23T01:15:53Z

@PHILO-HE @Yuhta @oerling We also did the performance test on our side. We did not observe the regression. Also, I agrees with @oerling ‘s opinion. Even though there is a regression, reading page headers should only account a very small portion and is not able to make a 30% E2E regression.

@PHILO-HE can you please double check you benchmark testing?

Thanks!

PHILO-HE · 2023-04-06T05:55:39Z

Thanks to everyone for your comment!
I tried with velox tpch benchmark, yes there is no perf. regression on my side also. Previously, we tested on spark + velox with modified tpch and dataset with date type replaced by string type. Not sure whether the test differences matter. I will give you feedback if it is reproducible. Thanks again!

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 22, 2023

liushengxuan changed the title ~~Use adaptive approach to determine the page header size~~ Use adaptive approach to determine the Parquet page header size Feb 22, 2023

liushengxuan force-pushed the shengxuan/fix_read_page_header_length branch 7 times, most recently from 225da2d to eaf2554 Compare February 22, 2023 06:20

Yuhta self-requested a review February 22, 2023 16:10

Yuhta linked an issue Feb 22, 2023 that may be closed by this pull request

Parquet PageReader reads the buffer with the incorrect length #3882

Closed

Yuhta mentioned this pull request Feb 22, 2023

Fix Incorrect Page Header Size Check for Parquet Page V1 #4016

Closed

yingsu00 reviewed Feb 24, 2023

View reviewed changes

velox/dwio/parquet/reader/PageReader.cpp Outdated Show resolved Hide resolved

velox/dwio/parquet/reader/PageReader.cpp Outdated Show resolved Hide resolved

Yuhta reviewed Feb 24, 2023

View reviewed changes

liushengxuan mentioned this pull request Mar 1, 2023

Refactor Thrift Transport for Parquet Metadata Access #4160

Closed

PHILO-HE mentioned this pull request Mar 2, 2023

[VL] bug of parquet read apache/incubator-gluten#1025

Closed

liushengxuan force-pushed the shengxuan/fix_read_page_header_length branch 4 times, most recently from 52b866b to 9754282 Compare March 2, 2023 20:31

liushengxuan changed the title ~~Use adaptive approach to determine the Parquet page header size~~ Read Parquet Page Header with ThriftStreamingTransport to Fix the Incorrect Header Length Mar 2, 2023

liushengxuan force-pushed the shengxuan/fix_read_page_header_length branch 2 times, most recently from a9265db to 5ebe4cd Compare March 3, 2023 07:50

Yuhta reviewed Mar 3, 2023

View reviewed changes

velox/dwio/parquet/reader/PageReader.h Outdated Show resolved Hide resolved

velox/dwio/parquet/reader/PageReader.cpp Outdated Show resolved Hide resolved

liushengxuan force-pushed the shengxuan/fix_read_page_header_length branch 3 times, most recently from b757240 to d346e91 Compare March 6, 2023 18:44

Yuhta reviewed Mar 6, 2023

View reviewed changes

velox/dwio/parquet/reader/PageReader.cpp Outdated Show resolved Hide resolved

liushengxuan closed this Mar 6, 2023

liushengxuan force-pushed the shengxuan/fix_read_page_header_length branch from d346e91 to 4ad9ed9 Compare March 6, 2023 23:11

liushengxuan reopened this Mar 6, 2023

liushengxuan force-pushed the shengxuan/fix_read_page_header_length branch from 0e1a458 to 101a05f Compare March 6, 2023 23:17

PHILO-HE added a commit to PHILO-HE/velox that referenced this pull request Mar 7, 2023

Port a patch: Read Parquet Page Header with ThriftStreamingTransport …

9922f4c

…to Fix the Incorrect Header Length facebookincubator#4108

facebook-github-bot closed this in 94905a0 Mar 7, 2023

facebook-github-bot added the Merged label Mar 7, 2023

Yuhta reviewed Mar 21, 2023

View reviewed changes

Yuhta mentioned this pull request Mar 21, 2023

Parquet PageReader reads the buffer with the incorrect length #3882

Closed

Yuhta reviewed Mar 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read Parquet Page Header with ThriftStreamingTransport to Fix the Incorrect Header Length #4108

Read Parquet Page Header with ThriftStreamingTransport to Fix the Incorrect Header Length #4108

liushengxuan commented Feb 22, 2023 •

edited

Loading

netlify bot commented Feb 22, 2023 •

edited

Loading

liushengxuan commented Feb 22, 2023

facebook-github-bot commented Mar 6, 2023

facebook-github-bot commented Mar 7, 2023

PHILO-HE commented Mar 21, 2023

Yuhta commented Mar 21, 2023

Yuhta Mar 21, 2023

Yuhta Mar 21, 2023

Yuhta Mar 21, 2023

Yuhta commented Mar 22, 2023

Yuhta commented Mar 22, 2023

oerling commented Mar 22, 2023 via email

liushengxuan commented Mar 23, 2023

PHILO-HE commented Apr 6, 2023

Read Parquet Page Header with ThriftStreamingTransport to Fix the Incorrect Header Length #4108

Read Parquet Page Header with ThriftStreamingTransport to Fix the Incorrect Header Length #4108

Conversation

liushengxuan commented Feb 22, 2023 • edited Loading

netlify bot commented Feb 22, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

liushengxuan commented Feb 22, 2023

facebook-github-bot commented Mar 6, 2023

facebook-github-bot commented Mar 7, 2023

PHILO-HE commented Mar 21, 2023

Yuhta commented Mar 21, 2023

Yuhta Mar 21, 2023

Choose a reason for hiding this comment

Yuhta Mar 21, 2023

Choose a reason for hiding this comment

Yuhta Mar 21, 2023

Choose a reason for hiding this comment

Yuhta commented Mar 22, 2023

Yuhta commented Mar 22, 2023

oerling commented Mar 22, 2023 via email

liushengxuan commented Mar 23, 2023

PHILO-HE commented Apr 6, 2023

liushengxuan commented Feb 22, 2023 •

edited

Loading

netlify bot commented Feb 22, 2023 •

edited

Loading