Skip to content

PARQUET-1478: Can't read spec compliant, 3-level lists via parquet-proto #578

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

Merged
zivanfi merged 4 commits into from
Jan 7, 2019
Merged

PARQUET-1478: Can't read spec compliant, 3-level lists via parquet-proto #578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
abf74a9
PARQUET-1478: Can't read spec compliant, 3-level lists via parquet-proto
nkollar Dec 14, 2018
f22296e
Fix list/map read path for parquet-proto
nkollar Dec 14, 2018
0d51536
Revert "Fix list/map read path for parquet-proto"
nkollar Dec 29, 2018
90be23b
Use suggested way from review to fix the failure
nkollar Dec 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion parquet-protobuf/src/main/java/org/apache/parquet/proto/ProtoMessageConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ public Optional<Converter> visit(LogicalTypeAnnotation.ListLogicalTypeAnnotation
public Optional<Converter> visit(LogicalTypeAnnotation.MapLogicalTypeAnnotation mapLogicalType) {
return of(new MapConverter(parentBuilder, fieldDescriptor, parquetType));
}
}).orElse(newScalarConverter(parent, parentBuilder, fieldDescriptor, parquetType));
}).orElseGet(() -> newScalarConverter(parent, parentBuilder, fieldDescriptor, parquetType));
}
Copy link
Contributor

@qinghui-xu qinghui-xu Dec 27, 2018

Choose a reason for hiding this comment

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

In fact, the root cause is we use orElse here, instead of orElseGet.
orElseGet is the lazy way to get the fallback value, while orElse is not. In the case of repeated fields (list and map), it was still trying to call newScalarConverter while using orELse, which is irrelevant (and thus the bug).

Copy link
Contributor Author

@nandorKollar nandorKollar Dec 29, 2018

Choose a reason for hiding this comment

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

Thanks @qinghui-xu , looks like the fix you suggested works well!

In this case, the good news: this is a bug, which is not yet released, because that orElse was added by me in PARQUET-1410 (my bad). Bad news: it is a regression, would be nice to get this committed before next release.

Copy link
Contributor

@qinghui-xu qinghui-xu Jan 2, 2019

Choose a reason for hiding this comment

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

Thanks for the prompt fix.


private Converter newScalarConverter(ParentValueContainer pvc, Message.Builder parentBuilder, Descriptors.FieldDescriptor fieldDescriptor, Type parquetType) {
Expand Down
4 changes: 2 additions & 2 deletions parquet-protobuf/src/test/java/org/apache/parquet/proto/ProtoInputOutputFormatTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ public void testMapIntMessageClassSchemaCompliant() throws Exception {
ProtoWriteSupport.setWriteSpecsCompliant(conf, true);

Path outputPath = new WriteUsingMR(conf).write(msgEmpty, msgNonEmpty);
ReadUsingMR readUsingMR = new ReadUsingMR();
ReadUsingMR readUsingMR = new ReadUsingMR(conf);
String customClass = TestProtobuf.MapIntMessage.class.getName();
ProtoReadSupport.setProtobufClass(readUsingMR.getConfiguration(), customClass);
List<Message> result = readUsingMR.read(outputPath);
Expand Down Expand Up @@ -303,7 +303,7 @@ public void testRepeatedInnerMessageClassSchemaCompliant() throws Exception {
ProtoWriteSupport.setWriteSpecsCompliant(conf, true);

Path outputPath = new WriteUsingMR(conf).write(msgEmpty, msgNonEmpty);
ReadUsingMR readUsingMR = new ReadUsingMR();
ReadUsingMR readUsingMR = new ReadUsingMR(conf);
String customClass = TestProtobuf.RepeatedInnerMessage.class.getName();
ProtoReadSupport.setProtobufClass(readUsingMR.getConfiguration(), customClass);
List<Message> result = readUsingMR.read(outputPath);
Expand Down
19 changes: 13 additions & 6 deletions parquet-protobuf/src/test/java/org/apache/parquet/proto/utils/ReadUsingMR.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
/*
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
*
* http://www.apache.org/licenses/LICENSE-2.0
*
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
Expand All @@ -28,7 +28,6 @@
import org.apache.hadoop.mapreduce.lib.output.NullOutputFormat;
import org.apache.parquet.proto.ProtoParquetInputFormat;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
Expand All @@ -41,19 +40,27 @@ public class ReadUsingMR {

private static List<Message> outputMessages;

Configuration conf = new Configuration();
Configuration conf;
private String projection;

public void setRequestedProjection(String projection) {
this.projection = projection;
}

public ReadUsingMR() {
this(new Configuration());
}

public ReadUsingMR(Configuration conf) {
this.conf = conf;
}

public Configuration getConfiguration() {
return conf;
}

public static class ReadingMapper extends Mapper<Void, MessageOrBuilder, LongWritable, Message> {
protected void map(Void key, MessageOrBuilder value, Context context) throws IOException, InterruptedException {
protected void map(Void key, MessageOrBuilder value, Context context) {
Message clone = ((Message.Builder) value).build();
outputMessages.add(clone);
}
Expand Down
9 changes: 4 additions & 5 deletions parquet-protobuf/src/test/java/org/apache/parquet/proto/utils/WriteUsingMR.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
/*
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
*
* http://www.apache.org/licenses/LICENSE-2.0
*
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
Expand All @@ -33,7 +33,6 @@
import org.slf4j.LoggerFactory;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
Expand All @@ -55,7 +54,7 @@ public WriteUsingMR() {
}

public WriteUsingMR(Configuration conf) {
this.conf = new Configuration();
this.conf = conf;
Copy link
Contributor

@qinghui-xu qinghui-xu Dec 27, 2018

Choose a reason for hiding this comment

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

Nice catch!

}

public Configuration getConfiguration() {
Expand Down