Skip to content

PARQUET-660: Ignore extension fields in protobuf messages. #351

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

Closed
jkukul wants to merge 3 commits into from
Closed

PARQUET-660: Ignore extension fields in protobuf messages. #351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e910a8a
PARQUET-660: Refactor: Add missing indentation.
jkukul Jul 9, 2016
db6e08b
PARQUET-660: Refactor: Don't use additional variable for indexing fie…
jkukul Jul 9, 2016
27580ab
PARQUET-660: Throw Unsupported exception for messages with extensions.
jkukul Jul 10, 2016
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
30 changes: 15 additions & 15 deletions parquet-protobuf/src/main/java/org/apache/parquet/proto/ProtoSchemaConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,21 +86,21 @@ private <T> Builder<? extends Builder<?, GroupBuilder<T>>, GroupBuilder<T>> addF
Type.Repetition repetition = getRepetition(descriptor);
JavaType javaType = descriptor.getJavaType();
switch (javaType) {
case BOOLEAN : return builder.primitive(BOOLEAN, repetition);
case INT : return builder.primitive(INT32, repetition);
case LONG : return builder.primitive(INT64, repetition);
case FLOAT : return builder.primitive(FLOAT, repetition);
case DOUBLE: return builder.primitive(DOUBLE, repetition);
case BYTE_STRING: return builder.primitive(BINARY, repetition);
case STRING: return builder.primitive(BINARY, repetition).as(UTF8);
case MESSAGE: {
GroupBuilder<GroupBuilder<T>> group = builder.group(repetition);
convertFields(group, descriptor.getMessageType().getFields());
return group;
}
case ENUM: return builder.primitive(BINARY, repetition).as(ENUM);
default:
throw new UnsupportedOperationException("Cannot convert Protocol Buffer: unknown type " + javaType);
case BOOLEAN: return builder.primitive(BOOLEAN, repetition);
case INT: return builder.primitive(INT32, repetition);
case LONG: return builder.primitive(INT64, repetition);
case FLOAT: return builder.primitive(FLOAT, repetition);
case DOUBLE: return builder.primitive(DOUBLE, repetition);
case BYTE_STRING: return builder.primitive(BINARY, repetition);
case STRING: return builder.primitive(BINARY, repetition).as(UTF8);
case MESSAGE: {
GroupBuilder<GroupBuilder<T>> group = builder.group(repetition);
convertFields(group, descriptor.getMessageType().getFields());
return group;
}
case ENUM: return builder.primitive(BINARY, repetition).as(ENUM);
default:
throw new UnsupportedOperationException("Cannot convert Protocol Buffer: unknown type " + javaType);
}
}

Expand Down
13 changes: 9 additions & 4 deletions parquet-protobuf/src/main/java/org/apache/parquet/proto/ProtoWriteSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,6 @@ class MessageWriter extends FieldWriter {
List<Descriptors.FieldDescriptor> fields = descriptor.getFields();
fieldWriters = (FieldWriter[]) Array.newInstance(FieldWriter.class, fields.size());

int i = 0;
for (Descriptors.FieldDescriptor fieldDescriptor: fields) {
String name = fieldDescriptor.getName();
Type type = schema.getType(name);
Expand All @@ -169,8 +168,7 @@ class MessageWriter extends FieldWriter {
writer.setFieldName(name);
writer.setIndex(schema.getFieldIndex(name));

fieldWriters[i] = writer;
i++;
fieldWriters[fieldDescriptor.getIndex()] = writer;
Copy link
Member

@julienledem julienledem Aug 1, 2016

Choose a reason for hiding this comment

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

is this the same change?

Copy link
Contributor Author

@jkukul jkukul Aug 5, 2016

Choose a reason for hiding this comment

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

Yes, fields in the list over which we're iterating are ordered by index. Here's how the list is built:
https://github.com/google/protobuf/blob/master/java/core/src/main/java/com/google/protobuf/Descriptors.java#L826

}
}

Expand Down Expand Up @@ -220,6 +218,13 @@ private void writeAllFields(MessageOrBuilder pb) {

for (Map.Entry<Descriptors.FieldDescriptor, Object> entry : changedPbFields.entrySet()) {
Descriptors.FieldDescriptor fieldDescriptor = entry.getKey();

if(fieldDescriptor.isExtension()) {
// Field index of an extension field might overlap with a base field.
throw new UnsupportedOperationException(
"Cannot convert Protobuf message with extension field(s)");
}

Copy link
Contributor Author

@jkukul jkukul Jul 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Another option would be throw an Exception here and explicitly do not support messages with extensions.

I could also log a warning here, somehow only once though, to avoid the noise.

Copy link
Member

@julienledem julienledem Jul 13, 2016

Choose a reason for hiding this comment

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

I like the approach of throwing an exception. It is easy to add support for a special case later. It is harder to change behavior as it breaks existing applications.
For example in Avro we explicitly throw in the case of recursive schemas.

int fieldIndex = fieldDescriptor.getIndex();
fieldWriters[fieldIndex].writeField(entry.getValue());
}
Expand Down Expand Up @@ -276,7 +281,7 @@ final void writeRawValue(Object value) {
}

class IntWriter extends FieldWriter {
@Override
@Override
final void writeRawValue(Object value) {
recordConsumer.addInteger((Integer) value);
}
Expand Down
15 changes: 15 additions & 0 deletions parquet-protobuf/src/test/java/org/apache/parquet/proto/ProtoWriteSupportTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,4 +165,19 @@ public void testOptionalInnerMessage() throws Exception {
inOrder.verify(readConsumerMock).endMessage();
Mockito.verifyNoMoreInteractions(readConsumerMock);
}

@Test(expected = UnsupportedOperationException.class)
public void testMessageWithExtensions() throws Exception {
RecordConsumer readConsumerMock = Mockito.mock(RecordConsumer.class);
ProtoWriteSupport instance = createReadConsumerInstance(TestProtobuf.Vehicle.class, readConsumerMock);

TestProtobuf.Vehicle.Builder msg = TestProtobuf.Vehicle.newBuilder();
msg.setHorsePower(300);
// Currently there's no support for extension fields. This test tests that the extension field
// will cause an exception.
msg.setExtension(TestProtobuf.Airplane.wingSpan, 50);

instance.write(msg.build());
}

}
11 changes: 11 additions & 0 deletions parquet-protobuf/src/test/resources/TestProtobuf.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,3 +137,14 @@ message SecondCustomClassMessage {
}

//please place your unit test Protocol Buffer definitions here.

message Vehicle {
optional int32 horsePower = 1;
extensions 100 to 199;
}

message Airplane {
extend Vehicle {
optional int32 wingSpan = 101;
}
}