Skip to content

GH-38071: [C++][CI] Fix Overlap column chunk ranges for pre-buffer - #38073

Merged
kou merged 5 commits into
apache:mainfrom
mapleFU:parquet-dcheck-fix-on-lazy
Oct 8, 2023
Merged

GH-38071: [C++][CI] Fix Overlap column chunk ranges for pre-buffer#38073
kou merged 5 commits into
apache:mainfrom
mapleFU:parquet-dcheck-fix-on-lazy

Conversation

@mapleFU

@mapleFUmapleFU commented Oct 6, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Rationale for this change

The C++ Parquet Arrow fuzz will generate bad Parquet file with bad row-range, this patch change the CoalesceReadRanges to return Result<>.

What changes are included in this PR?

Just a checking, change CoalesceReadRanges to return Result<>.

Are these changes tested?

No.

Are there any user-facing changes?

No.

@github-actions

github-actionsBot commented Oct 6, 2023

Copy link
Copy Markdown

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

@mapleFU
mapleFU requested a review from pitrouOctober 6, 2023 12:26
@mapleFUmapleFU changed the title GH-38071: [C++] Fix Overlap column chunk ranges for pre-bufferGH-38071: [C++][CI] Fix Overlap column chunk ranges for pre-bufferOct 6, 2023
mapleFU
mapleFU commented Oct 6, 2023
View reviewed changes
Comment threadcpp/src/parquet/file_reader.cc Outdated
@github-actionsgithub-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 6, 2023
@mapleFU

mapleFU commented Oct 6, 2023

Copy link
Copy Markdown
MemberAuthor

@jorisvandenbossche@lidavidm I've add an Result<> as return value here

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Oct 6, 2023
mapleFU
mapleFU commented Oct 6, 2023
View reviewed changes
Comment threadcpp/src/arrow/io/util_internal.h
std::vector<ReadRange> CoalesceReadRanges(std::vector<ReadRange> ranges,
int64_t hole_size_limit,
int64_t range_size_limit);
Result<std::vector<ReadRange>> CoalesceReadRanges(std::vector<ReadRange> ranges,

@mapleFUmapleFUOct 6, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Don't know if this matter, since it change the public api.

@lidavidmlidavidmOct 6, 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.

It's in an internal header so no expectation that it remains stable

@mapleFUmapleFUOct 6, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Oh, that's good, thanks!

@mapleFU
mapleFU marked this pull request as ready for review October 6, 2023 15:05
@mapleFU
mapleFU requested a review from wgtmac as a code ownerOctober 6, 2023 15:05
@mapleFU

mapleFU commented Oct 6, 2023

Copy link
Copy Markdown
MemberAuthor
[ RUN ] AzureFileSystem.UploadThenDownload
unknown file: Failure
C++ exception with description "Fail to get a new connection for: http://127.0.0.1:10000./ Couldn't connect to server" thrown in the test body.
[ FAILED ] AzureFileSystem.UploadThenDownload (6053 ms)

Hmm...Seems this is not caused by me 🤔

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Oct 6, 2023
kou
kou approved these changes Oct 7, 2023
View reviewed changes

@koukou left a comment

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.

+1

The Azure failure will be unrelated. I re-ran the job.

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Oct 7, 2023
@mapleFU

mapleFU commented Oct 8, 2023

Copy link
Copy Markdown
MemberAuthor

I've rebase the master to rerun the test :-) Hope this time it passes...

@kou

kou commented Oct 8, 2023

Copy link
Copy Markdown
Member

Passed. :-)
I'll merge this.

@kou
kou merged commit 0b9f817 into apache:mainOct 8, 2023
@koukou removed the awaiting merge Awaiting merge label Oct 8, 2023
@conbench-apache-arrow

conbench-apache-arrowBot commented Oct 9, 2023

Copy link
Copy Markdown

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@mapleFU
mapleFU deleted the parquet-dcheck-fix-on-lazy branch October 9, 2023 02:12
@jorisvandenbossche

jorisvandenbossche commented Oct 9, 2023

Copy link
Copy Markdown
Member

Thanks a lot @mapleFU for fixing this!

@jorisvandenbosschejorisvandenbossche mentioned this pull request Oct 24, 2023
Closed
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
@mapleFU@loicalleyne
apacheGH-38071: [C++][CI] Fix Overlap column chunk ranges for pre-buf…
aadb69c
…fer (apache#38073)
### Rationale for this change
The C++ Parquet Arrow fuzz will generate bad Parquet file with bad row-range, this patch change the `CoalesceReadRanges` to return `Result<>`.
### What changes are included in this PR?
Just a checking, change `CoalesceReadRanges` to return `Result<>`.
### Are these changes tested?
No.
### Are there any user-facing changes?
No.
* Closes: apache#38071
Authored-by: mwish <maplewish117@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
@mapleFU@dgreiss
apacheGH-38071: [C++][CI] Fix Overlap column chunk ranges for pre-buf…
44d7e13
…fer (apache#38073)
### Rationale for this change
The C++ Parquet Arrow fuzz will generate bad Parquet file with bad row-range, this patch change the `CoalesceReadRanges` to return `Result<>`.
### What changes are included in this PR?
Just a checking, change `CoalesceReadRanges` to return `Result<>`.
### Are these changes tested?
No.
### Are there any user-facing changes?
No.
* Closes: apache#38071
Authored-by: mwish <maplewish117@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm approved these changes
@koukoukou approved these changes
@pitroupitrouAwaiting requested review from pitroupitrou is a code owner
@wgtmacwgtmacAwaiting requested review from wgtmacwgtmac is a code owner

Assignees

No one assigned

Labels

Component: C++Component: Parquet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[C++] Enabling CacheOptions::LazyDefault caused Parquet fuzzing failure

4 participants

@mapleFU@kou@jorisvandenbossche@lidavidm