Skip to content

Parquet: Fix map projection after map to key_value rename #3309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
rdblue merged 2 commits into from
Oct 19, 2021
Merged

Parquet: Fix map projection after map to key_value rename #3309

rdblue merged 2 commits into from
Oct 19, 2021

Conversation

@rdblue
Copy link
Contributor

@rdblue rdblue commented Oct 18, 2021 •
edited
Loading

This fixes the Parquet map projection bug introduced by apache/parquet-java#798

The projection code in Iceberg would create map projections by using the Parquet Types.map builder. But, the type created by this builder changed by renaming the key-value pair, map to key_value, so the projection was no longer valid for Parquet files. As a result, Parquet would not project the map column and loading it would fail with an error like this:

Caused by: java.lang.IllegalArgumentException: [mapCol, map, key] required binary key (STRING) = 2 is not in the store: [] 1000
        at org.apache.iceberg.shaded.org.apache.parquet.hadoop.ColumnChunkPageReadStore.getPageReader(ColumnChunkPageReadStore.java:272)
        at org.apache.iceberg.parquet.ParquetValueReaders$PrimitiveReader.setPageSource(ParquetValueReaders.java:185)
        at org.apache.iceberg.parquet.ParquetValueReaders$RepeatedKeyValueReader.setPageSource(ParquetValueReaders.java:529)
        at org.apache.iceberg.parquet.ParquetValueReaders$StructReader.setPageSource(ParquetValueReaders.java:685)

The solution is to copy the map structure and ensure that the names are preserved rather than generated.

Closes #2962.

@rdblue rdblue linked an issue Oct 18, 2021 that may be closed by this pull request
Parquet 1.11.1 update causes regressions while reading iceberg data written with v1.11.0 #2962
Closed
RussellSpitzer
RussellSpitzer approved these changes Oct 18, 2021
View reviewed changes
Copy link
Member

@RussellSpitzer RussellSpitzer left a comment

Choose a reason for hiding this comment

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

LGTM, Is there a non painful way we can add a test for this? Seems like manually testing it requires loading up a Parquet Version which defines map types with slightly different names?

@rdblue
Copy link
Contributor Author

rdblue commented Oct 18, 2021

We can add a test for PruneColumns directly that uses an alternative map structure. The only issue is that I had some trouble building the map type without going through the Types API. But there's probably a different way to do it that I was missing.

@rdblue rdblue added this to the Java 0.12.1 Release milestone Oct 18, 2021
@rdblue
Copy link
Contributor Author

rdblue commented Oct 19, 2021

Added the missing tests. I'll merge this when tests are passing.

RussellSpitzer
RussellSpitzer reviewed Oct 19, 2021
View reviewed changes
parquet/src/test/java/org/apache/iceberg/parquet/TestPruneColumns.java
.addField(Types.primitive(PrimitiveTypeName.DOUBLE, Type.Repetition.REQUIRED).id(5).named("y"))
.addField(Types.primitive(PrimitiveTypeName.DOUBLE, Type.Repetition.REQUIRED).id(6).named("z"))
.id(3)
.named("value"))
Copy link
Member

@RussellSpitzer RussellSpitzer Oct 19, 2021

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@kbendick kbendick Oct 19, 2021

Choose a reason for hiding this comment

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

This is quite the type declaration. Love it.

kbendick
kbendick approved these changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@kbendick kbendick left a comment

Choose a reason for hiding this comment

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

Great work @rdblue.

parquet/src/test/java/org/apache/iceberg/parquet/TestPruneColumns.java
.addField(Types.primitive(PrimitiveTypeName.DOUBLE, Type.Repetition.REQUIRED).id(5).named("y"))
.addField(Types.primitive(PrimitiveTypeName.DOUBLE, Type.Repetition.REQUIRED).id(6).named("z"))
.id(3)
.named("value"))
Copy link
Contributor

@kbendick kbendick Oct 19, 2021

Choose a reason for hiding this comment

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

This is quite the type declaration. Love it.

parquet/src/test/java/org/apache/iceberg/parquet/TestPruneColumns.java
}

@Test
public void testListElementName() {
Copy link
Contributor

@kbendick kbendick Oct 19, 2021

Choose a reason for hiding this comment

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

Maybe testListElementDoesNotAssumeName?

Copy link
Contributor Author

@rdblue rdblue Oct 19, 2021

Choose a reason for hiding this comment

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

If I were making another change I'd probably update this, but I think it's pretty minor and I'd like to get this in to make it possible to release 0.12.1.

@kbendick
Copy link
Contributor

kbendick commented Oct 19, 2021

Added the missing tests. I'll merge this when tests are passing.

These are great tests and this is great work. Thank you!

@rdblue rdblue merged commit edc6985 into apache:master Oct 19, 2021
@rdblue rdblue mentioned this pull request Oct 19, 2021
Closed
kbendick pushed a commit to kbendick/iceberg that referenced this pull request Oct 27, 2021
@rdblue @kbendick
Parquet: Fix map projection after map to key_value rename (apache#3309)
1525b94
kbendick pushed a commit to kbendick/iceberg that referenced this pull request Oct 27, 2021
@rdblue @kbendick
Parquet: Fix map projection after map to key_value rename (apache#3309)
0f64158
kbendick pushed a commit to kbendick/iceberg that referenced this pull request Oct 28, 2021
@rdblue @kbendick
Parquet: Fix map projection after map to key_value rename (apache#3309)
96030e0
rdblue added a commit that referenced this pull request Oct 29, 2021
@rdblue
Parquet: Fix map projection after map to key_value rename (#3309)
a7859dc
izchen pushed a commit to izchen/iceberg that referenced this pull request Dec 7, 2021
@rdblue
Parquet: Fix map projection after map to key_value rename (apache#3309)
050b32a
@kbendick kbendick mentioned this pull request Dec 13, 2021
Merged
Initial-neko pushed a commit to Initial-neko/iceberg that referenced this pull request Dec 15, 2021
@miniFixer
Parquet: Fix map projection after map to key_value rename apache#3309
9c306d4
Initial-neko pushed a commit to Initial-neko/iceberg that referenced this pull request Dec 17, 2021
@miniFixer
Parquet: Fix map projection after map to key_value rename apache#3309
a27c6d4
@islamismailov islamismailov mentioned this pull request May 26, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RussellSpitzer RussellSpitzer RussellSpitzer approved these changes

+1 more reviewer

@kbendick kbendick kbendick approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

parquet

Projects

None yet

Milestone

Java 0.12.1 Release

Development

Successfully merging this pull request may close these issues.

Parquet 1.11.1 update causes regressions while reading iceberg data written with v1.11.0

3 participants

@rdblue @kbendick @RussellSpitzer