Skip to content

Read parquet column chunks in small steps#15374

Merged
raunaqmorarka merged 6 commits intotrinodb:masterfrom
starburstdata:ls/054-parquet-small-buffer
Dec 23, 2022
Merged

Read parquet column chunks in small steps#15374
raunaqmorarka merged 6 commits intotrinodb:masterfrom
starburstdata:ls/054-parquet-small-buffer

Conversation

@lukasz-stec
Copy link
Copy Markdown
Member

@lukasz-stec lukasz-stec commented Dec 12, 2022 •
edited by raunaqmorarka
Loading

Description

Currently, ParquetReader will read the entire row group at once. This means that:

  • limit N queries will take longer time than needed because the reader might read hundreds of megabytes before returning a few rows.

  • Reader might allocate and read hundreds of megabytes at once. This is problematic because:

    • memory accounting for the reader happens after data is already read and arrays are created. This can lead to OOM and general system instability (e.g. GC) because such allocations will not be accounted for
    • peak memory usage is higher than needed (e.g. GBs vs MBs)

The issue can especially be seen when Parquet row group size is large.

This PR fixes this by reading parquet column chunks in small (8MB by default) pieces.

Additional context and related issues

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( X) Release notes are required, with the following suggested text:

# Hive, Delta, Hudi, Iceberg
* Avoid large memory allocations in parquet reader by limiting the maximum size of reads from file. This improves stability and reduces peak memory requirements. The catalog configuration property `parquet.max-buffer-size` can be used to change the maximum size of reads performed by the parquet reader from the default value of 8MB. ({issue}`15374`)

@cla-bot cla-bot bot added the cla-signed label Dec 12, 2022
@lukasz-stec lukasz-stec force-pushed the ls/054-parquet-small-buffer branch 5 times, most recently from fb2380e to 8c1f314 Compare December 13, 2022 09:50
@lukasz-stec lukasz-stec force-pushed the ls/054-parquet-small-buffer branch from 8c1f314 to 5972765 Compare December 13, 2022 11:03
raunaqmorarka
raunaqmorarka reviewed Dec 13, 2022
View reviewed changes
@lukasz-stec lukasz-stec force-pushed the ls/054-parquet-small-buffer branch from 5972765 to 24af6be Compare December 13, 2022 13:44
@lukasz-stec lukasz-stec force-pushed the ls/054-parquet-small-buffer branch 2 times, most recently from 1b732cf to 973e7e0 Compare December 15, 2022 08:51
@lukasz-stec lukasz-stec marked this pull request as ready for review December 15, 2022 08:52
@lukasz-stec
Copy link
Copy Markdown
Member Author

lukasz-stec commented Dec 15, 2022

I added tests for PageReader and ChunkedInputStream. This is ready for review.

lukasz-stec
lukasz-stec commented Dec 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec left a comment

Choose a reason for hiding this comment

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

some comments.

raunaqmorarka
raunaqmorarka reviewed Dec 15, 2022
View reviewed changes
@lukasz-stec lukasz-stec force-pushed the ls/054-parquet-small-buffer branch from 973e7e0 to 21de362 Compare December 15, 2022 15:52
lukasz-stec
lukasz-stec commented Dec 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec left a comment

Choose a reason for hiding this comment

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

most comments addressed

@lukasz-stec lukasz-stec force-pushed the ls/054-parquet-small-buffer branch from 21de362 to 90dbd07 Compare December 15, 2022 16:31
raunaqmorarka
raunaqmorarka reviewed Dec 15, 2022
View reviewed changes
@lukasz-stec lukasz-stec force-pushed the ls/054-parquet-small-buffer branch from 90dbd07 to 20e0a82 Compare December 16, 2022 10:35
lukasz-stec
lukasz-stec commented Dec 16, 2022
View reviewed changes
Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec left a comment

Choose a reason for hiding this comment

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

most comments addressed (javadoc for ChunkedInputStream still pending)

@lukasz-stec lukasz-stec force-pushed the ls/054-parquet-small-buffer branch from b90c5b1 to 8ae0089 Compare December 20, 2022 14:20
lukasz-stec
lukasz-stec commented Dec 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec left a comment

Choose a reason for hiding this comment

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

comments addressed. Test of memory accounting are pending

@lukasz-stec lukasz-stec force-pushed the ls/054-parquet-small-buffer branch from 8ae0089 to d28cd54 Compare December 20, 2022 15:08
@lukasz-stec
Copy link
Copy Markdown
Member Author

lukasz-stec commented Dec 20, 2022

memory accounting test added in io.trino.parquet.reader.TestParquetDataSource#testMemoryAccounting

@lukasz-stec
Move TestParquetDataSource to trino-parquet
5c64bce 
TestParquetDataSource tests only a generic AbstractParquetDataSource
logic so it should be in trino-parquet.
@lukasz-stec
Read parquet column chunks in small steps
7be2201 
Before this change, parquet column chunks were
read in one go, copying everything into one big Slice.
This had two issues.
One, for limit queries, we potentially don't need to
read entire column chunk to finish the query as first
page may satisfy the limit.
Second, for files with big row group size the allocated
Slice can exceed the jvm limits for native byte array,
and even if it doesn't, it makes memory usage not efficient
due to how humungous allocations are implemented in the jvm.
@lukasz-stec lukasz-stec force-pushed the ls/054-parquet-small-buffer branch from d28cd54 to 7be2201 Compare December 20, 2022 17:09
raunaqmorarka
raunaqmorarka approved these changes Dec 21, 2022
View reviewed changes
@lukasz-stec
Support reading large parquet column chunks
b581c52 
We can allow for a big DiskRange to be
passed to the ParquetDataSource.planRead,
since it's going to split the ranges into
small chunks anyway.
@lukasz-stec lukasz-stec force-pushed the ls/054-parquet-small-buffer branch from 5914833 to b581c52 Compare December 21, 2022 11:13
@lukasz-stec lukasz-stec requested a review from Dith3r December 21, 2022 11:13
@martint martint dismissed their stale review December 21, 2022 18:53

Comments were addressed. Thanks @lukasz-stec, @raunaqmorarka

@lukasz-stec
Copy link
Copy Markdown
Member Author

lukasz-stec commented Dec 22, 2022

Benchmark results for tpch/tpcds parquet sf1k partitioned with default parquet.max-buffer-size=8MB and parquet.max-buffer-size=128MB.

image

parquet-small-chunks-128MB-buffer-oss-sf1k-part-ext.pdf

@raunaqmorarka raunaqmorarka merged commit b107bf7 into trinodb:master Dec 23, 2022
@raunaqmorarka raunaqmorarka deleted the ls/054-parquet-small-buffer branch December 23, 2022 03:20
@github-actions github-actions bot added this to the 404 milestone Dec 23, 2022
This was referenced Dec 23, 2022
Closed
Merged
@colebow colebow mentioned this pull request Dec 28, 2022
Merged
This was referenced Dec 29, 2022
Merged
Closed
@raunaqmorarka raunaqmorarka mentioned this pull request Feb 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@raunaqmorarka raunaqmorarka raunaqmorarka approved these changes

@martint martint martint left review comments

@sopel39 sopel39 Awaiting requested review from sopel39

@radek-kondziolka radek-kondziolka Awaiting requested review from radek-kondziolka

+1 more reviewer

@Dith3r Dith3r Dith3r approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed performance

Milestone

405

Development

Successfully merging this pull request may close these issues.

ArithmeticException: integer overflow when querying large Parquet files with large row group (2GB)

4 participants

@lukasz-stec @martint @raunaqmorarka @Dith3r