[PARQUET-99] Page size check properties by danielcweeks · Pull Request #250 · apache/parquet-java

danielcweeks · 2015-07-24T23:48:17Z

Added property to toggle page size check estimation and initial row size checking.

…ize checking

danielcweeks · 2015-07-28T16:25:59Z

@rdblue could you do a quick review of this? I think you were most recently in this path for the padding feature. I've actually had confirmation from another company that this fixed their OOM issues, so we're not the only ones affected.

rdblue · 2015-07-28T17:42:20Z

@danielcweeks, I'll have a look at it today.

rdblue · 2015-07-28T18:34:38Z

Why are there only changes to ColumnWriteStoreV1 or ColumnWriterV1? Does V2 not need to be changed?

I'll take a look at the v2 path. It's changed from the v1 path and I'm not sure if the logic is the same.

geovations · 2015-07-29T19:23:37Z

We were seeing this issue with very wide columns of text. While writing the parquet to disk, the writer would run out of memory. Once we applied this patch to v1.8.1, the memory issues were a thing of the past.

phoenixhadoop · 2015-11-26T15:20:16Z

I met https://issues.apache.org/jira/browse/PARQUET-99.
When would you fix this bug?
I need it .Thank you.

danielcweeks · 2015-11-30T17:30:06Z

@phoenixhadoop I'm working on rebasing this to master now. Hopefully it will make the next release.

danielcweeks · 2015-12-01T18:34:46Z

@rdblue I've updated the pull request to include both v1 and v2 writers. This new approach preserves the behavior of both readers by default and exposes the ability for you to set values for min and max row counts for page size checking (v1 only supports min as initial when estimate is disabled).

rdblue · 2015-12-01T18:45:16Z

Thanks! I'll take a look.

rdblue · 2015-12-02T17:52:55Z

I think this should be minRowCountForPageSizeCheck. It looks like the name is wrong, but the value passed in by ParquetProperties is the min.

Good catch. Actually, it should be "initial" for the sig. The issue is that v1 writer doesn't enforce a min or max and I wanted to maintain the same behavior. So v1 will use "min" as "initial" and v2 will support "min/max".

I'll fix the name.

rdblue · 2015-12-02T18:02:21Z

Nit: the rest of the codebase puts a space between if and the left paren.

rdblue · 2015-12-02T19:48:40Z

I really like how this uses ParquetProperties to carry the new settings. I think that's a step in the right direction. But, it looks like we haven't quite moved over to ParquetProperties in some important ways and that makes this need to add the 3 new settings all over the place. This is a bigger issue than just this fix, so I went ahead and did some cleanup and posted a work-in-progress pull request: #297

It's based on a previous version that doesn't have this morning's fixes. What do you think?

rdblue · 2015-12-02T19:56:44Z

This looks good to me now.

Nit: the comments aren't needed and reference values no longer used. Maybe just change the "will check again halfway" by adding " if it is between min and max"

julienledem · 2015-12-07T23:31:16Z

overall this looks good to me.

rdblue · 2015-12-08T22:48:36Z

I merged this in #297. Thanks for fixing this, Dan!

This adds properties to set the min and max number of records that are passed between page checks, as well as a property that controls whether the next check will be based on records already seen or set to the minimum number of records between checks. * `parquet.page.size.row.check.min` - minimum number of records between page size checks * `parquet.page.size.row.check.max` - maximum number of records between page size checks * `parquet.page.size.check.estimate` - whether to estimate the number of records before the next check, or to always use the minimum number of records. This also updates the internal API to use ParquetProperties to carry encoding settings (used in parquet-column) to reduce the number of parameters passed through internal APIs. It also adds a builder for ParquetProperties to avoid needing to reference defaults in other modules. This closes apache#250 Author: Daniel Weeks <dweeks@netflix.com> Author: Ryan Blue <blue@apache.org> Closes apache#297 from rdblue/parquet-properties-update and squashes the following commits: c93b73e [Ryan Blue] PARQUET-99: Use ParquetProperties to carry encoding config. 18f8d3a [Daniel Weeks] Spacing 2090719 [Daniel Weeks] Update sizeCheck to write page properly if estimating is turned off 71336ee [Daniel Weeks] Fixed param name 5d99072 [Daniel Weeks] Update page size checking for v2 writer 3f7870c [Daniel Weeks] Rebase to resolve byte buffer conflicts 68794f0 [Daniel Weeks] Merge branch 'master' into page_size_check b49f03c [Daniel Weeks] Fixed reset of nextSizeCheck a057f46 [Daniel Weeks] Fixed inverted property logic e7cd54b [Daniel Weeks] Added property to toggle page size check estimation and initial row size checking

This adds properties to set the min and max number of records that are passed between page checks, as well as a property that controls whether the next check will be based on records already seen or set to the minimum number of records between checks. * `parquet.page.size.row.check.min` - minimum number of records between page size checks * `parquet.page.size.row.check.max` - maximum number of records between page size checks * `parquet.page.size.check.estimate` - whether to estimate the number of records before the next check, or to always use the minimum number of records. This also updates the internal API to use ParquetProperties to carry encoding settings (used in parquet-column) to reduce the number of parameters passed through internal APIs. It also adds a builder for ParquetProperties to avoid needing to reference defaults in other modules. This closes apache#250 Author: Daniel Weeks <dweeks@netflix.com> Author: Ryan Blue <blue@apache.org> Closes apache#297 from rdblue/parquet-properties-update and squashes the following commits: c93b73e [Ryan Blue] PARQUET-99: Use ParquetProperties to carry encoding config. 18f8d3a [Daniel Weeks] Spacing 2090719 [Daniel Weeks] Update sizeCheck to write page properly if estimating is turned off 71336ee [Daniel Weeks] Fixed param name 5d99072 [Daniel Weeks] Update page size checking for v2 writer 3f7870c [Daniel Weeks] Rebase to resolve byte buffer conflicts 68794f0 [Daniel Weeks] Merge branch 'master' into page_size_check b49f03c [Daniel Weeks] Fixed reset of nextSizeCheck a057f46 [Daniel Weeks] Fixed inverted property logic e7cd54b [Daniel Weeks] Added property to toggle page size check estimation and initial row size checking Conflicts: parquet-column/src/main/java/org/apache/parquet/column/ParquetProperties.java parquet-column/src/main/java/org/apache/parquet/column/impl/ColumnWriteStoreV1.java parquet-column/src/main/java/org/apache/parquet/column/impl/ColumnWriteStoreV2.java parquet-column/src/main/java/org/apache/parquet/column/impl/ColumnWriterV1.java parquet-column/src/main/java/org/apache/parquet/column/impl/ColumnWriterV2.java parquet-column/src/test/java/org/apache/parquet/column/impl/TestCorruptDeltaByteArrays.java parquet-column/src/test/java/org/apache/parquet/column/mem/TestMemColumn.java parquet-column/src/test/java/org/apache/parquet/io/PerfTest.java parquet-column/src/test/java/org/apache/parquet/io/TestColumnIO.java parquet-column/src/test/java/org/apache/parquet/io/TestFiltered.java parquet-hadoop/src/main/java/org/apache/parquet/hadoop/InternalParquetRecordWriter.java parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetOutputFormat.java parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetRecordWriter.java parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetWriter.java parquet-pig/src/test/java/org/apache/parquet/pig/TupleConsumerPerfTest.java parquet-thrift/src/test/java/org/apache/parquet/thrift/TestParquetReadProtocol.java Resolution: Fixed changes that depended on the addition of an allocator argument Ignored adjacent changes that were flagged Passed page size at compressor instantiation instead of to the factory

Daniel Weeks added 2 commits July 24, 2015 16:44

Added property to toggle page size check estimation and initial row s…

e7cd54b

…ize checking

Fixed inverted property logic

a057f46

rdblue reviewed Jul 28, 2015
View reviewed changes

Fixed reset of nextSizeCheck

b49f03c

Daniel Weeks added 3 commits November 30, 2015 09:30

Merge branch 'master' into page_size_check

68794f0

Rebase to resolve byte buffer conflicts

3f7870c

Update page size checking for v2 writer

5d99072

rdblue reviewed Dec 2, 2015
View reviewed changes

Fixed param name

71336ee

rdblue reviewed Dec 2, 2015
View reviewed changes

Daniel Weeks added 2 commits December 2, 2015 10:10

Update sizeCheck to write page properly if estimating is turned off

2090719

Spacing

18f8d3a

rdblue reviewed Dec 2, 2015
View reviewed changes

rdblue mentioned this pull request Dec 5, 2015

PARQUET-99: Add page size check properties #297

Closed

rdblue mentioned this pull request Dec 8, 2015

PARQUET-353: Release compression resources. #295

Closed

asfgit closed this in 5632640 Dec 8, 2015

asfimport mentioned this pull request Jun 23, 2024

Large rows cause unnecessary OOM exceptions #1584

Closed

Uh oh!

Conversation

danielcweeks commented Jul 24, 2015

Uh oh!

danielcweeks commented Jul 28, 2015

Uh oh!

rdblue commented Jul 28, 2015

Uh oh!

rdblue Jul 28, 2015

Choose a reason for hiding this comment

Uh oh!

danielcweeks Jul 28, 2015

Choose a reason for hiding this comment

Uh oh!

geovations commented Jul 29, 2015

Uh oh!

phoenixhadoop commented Nov 26, 2015

Uh oh!

danielcweeks commented Nov 30, 2015

Uh oh!

danielcweeks commented Dec 1, 2015

Uh oh!

rdblue commented Dec 1, 2015

Uh oh!

rdblue Dec 2, 2015

Choose a reason for hiding this comment

Uh oh!

danielcweeks Dec 2, 2015

Choose a reason for hiding this comment

Uh oh!

danielcweeks Dec 2, 2015

Choose a reason for hiding this comment

Uh oh!

rdblue Dec 2, 2015

Choose a reason for hiding this comment

Uh oh!

rdblue commented Dec 2, 2015

Uh oh!

rdblue Dec 2, 2015

Choose a reason for hiding this comment

Uh oh!

julienledem commented Dec 7, 2015

Uh oh!

rdblue commented Dec 8, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants