Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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: 2 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,8 @@ project(':iceberg-flink') {
testCompile("org.apache.flink:flink-test-utils_2.12") {
exclude group: "org.apache.curator", module: 'curator-test'
}

testCompile project(path: ':iceberg-api', configuration: 'testArtifacts')
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
import java.util.Map;
import org.apache.iceberg.Schema;
import org.apache.iceberg.data.GenericRecord;
import org.apache.iceberg.data.Record;
import org.apache.iceberg.parquet.ParquetSchemaUtil;
import org.apache.iceberg.parquet.ParquetValueReader;
import org.apache.iceberg.parquet.ParquetValueReaders;
Expand Down Expand Up @@ -67,26 +66,34 @@ private GenericParquetReaders() {

public static ParquetValueReader<GenericRecord> buildReader(Schema expectedSchema,
MessageType fileSchema) {
return buildReader(expectedSchema, fileSchema, ImmutableMap.of());
return buildReader(expectedSchema, fileSchema, ImmutableMap.of(), RecordReader::new);
}
@SuppressWarnings("unchecked")

public static ParquetValueReader<GenericRecord> buildReader(Schema expectedSchema,
MessageType fileSchema,
Map<Integer, ?> idToConstant) {
return buildReader(expectedSchema, fileSchema, idToConstant, RecordReader::new);
}

@SuppressWarnings("unchecked")
public static <T> ParquetValueReader<T> buildReader(Schema expectedSchema,
MessageType fileSchema,
Map<Integer, ?> idToConstant,
StructReaderFactory<T> structReaderFactory) {
if (ParquetSchemaUtil.hasIds(fileSchema)) {
return (ParquetValueReader<GenericRecord>)
return (ParquetValueReader<T>)
TypeWithSchemaVisitor.visit(expectedSchema.asStruct(), fileSchema,
new ReadBuilder(fileSchema, idToConstant));
new ReadBuilder(fileSchema, idToConstant, structReaderFactory));
} else {
return (ParquetValueReader<GenericRecord>)
return (ParquetValueReader<T>)
TypeWithSchemaVisitor.visit(expectedSchema.asStruct(), fileSchema,
new FallbackReadBuilder(fileSchema, idToConstant));
new FallbackReadBuilder(fileSchema, idToConstant, structReaderFactory));
}
}

private static class FallbackReadBuilder extends ReadBuilder {
FallbackReadBuilder(MessageType type, Map<Integer, ?> idToConstant) {
super(type, idToConstant);
FallbackReadBuilder(MessageType type, Map<Integer, ?> idToConstant, StructReaderFactory structReaderFactory) {
super(type, idToConstant, structReaderFactory);
}

@Override
Expand All @@ -111,17 +118,19 @@ public ParquetValueReader<?> struct(StructType expected, GroupType struct,
types.add(fieldType);
}

return new RecordReader(types, newFields, expected);
return structReaderFactory().create(types, newFields, expected);
}
}

private static class ReadBuilder extends TypeWithSchemaVisitor<ParquetValueReader<?>> {
private final MessageType type;
private final Map<Integer, ?> idToConstant;
private final StructReaderFactory structReaderFactory;

ReadBuilder(MessageType type, Map<Integer, ?> idToConstant) {
ReadBuilder(MessageType type, Map<Integer, ?> idToConstant, StructReaderFactory structReaderFactory) {
this.type = type;
this.idToConstant = idToConstant;
this.structReaderFactory = structReaderFactory;
}

@Override
Expand Down Expand Up @@ -168,7 +177,7 @@ public ParquetValueReader<?> struct(StructType expected, GroupType struct,
}
}

return new RecordReader(types, reorderedFields, expected);
return structReaderFactory.create(types, reorderedFields, expected);
}

@Override
Expand Down Expand Up @@ -298,6 +307,10 @@ public ParquetValueReader<?> primitive(org.apache.iceberg.types.Type.PrimitiveTy
MessageType type() {
return type;
}

StructReaderFactory structReaderFactory() {
return structReaderFactory;
}
}

private static final OffsetDateTime EPOCH = Instant.ofEpochSecond(0).atOffset(ZoneOffset.UTC);
Expand Down Expand Up @@ -396,7 +409,12 @@ public byte[] read(byte[] reuse) {
}
}

static class RecordReader extends StructReader<Record, Record> {
public interface StructReaderFactory<T> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar to the write path, I think it would be nice to refactor this to avoid exposing new public methods and interfaces.


StructReader<T, T> create(List<Type> types, List<ParquetValueReader<?>> readers, StructType struct);
}

static class RecordReader extends StructReader<GenericRecord, GenericRecord> {

@rdblue rdblue Jun 24, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why change this to use the GenericRecord instance rather than the Record interface?

I don't see much value in this change. We will just need to change it back if we want to add implementations of Record that are not generic, like we do with our internal classes that extend Avro's IndexedRecord. Ideally, I'd like to change those over to use our generics readers eventually.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I changed this because the buildReader method return the GenericRecord ParquetValueReader. Seems better to change the ParquetValueReader to be Record reader.

private final StructType structType;

RecordReader(List<Type> types,
Expand All @@ -407,7 +425,7 @@ static class RecordReader extends StructReader<Record, Record> {
}

@Override
protected Record newStructData(Record reuse) {
protected GenericRecord newStructData(GenericRecord reuse) {
if (reuse != null) {
return reuse;
} else {
Expand All @@ -417,17 +435,17 @@ protected Record newStructData(Record reuse) {

@Override
@SuppressWarnings("unchecked")
protected Object getField(Record intermediate, int pos) {
protected Object getField(GenericRecord intermediate, int pos) {
return intermediate.get(pos);
}

@Override
protected Record buildStruct(Record struct) {
protected GenericRecord buildStruct(GenericRecord struct) {
return struct;
}

@Override
protected void set(Record struct, int pos, Object value) {
protected void set(GenericRecord struct, int pos, Object value) {
struct.set(pos, value);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,21 @@ private GenericParquetWriter() {

@SuppressWarnings("unchecked")
public static <T> ParquetValueWriter<T> buildWriter(MessageType type) {
return (ParquetValueWriter<T>) ParquetTypeVisitor.visit(type, new WriteBuilder(type));
return buildWriter(type, RecordWriter::new);
}

@SuppressWarnings("unchecked")
public static <T> ParquetValueWriter<T> buildWriter(MessageType type, StructWriterFactory structWriterFactory) {
return (ParquetValueWriter<T>) ParquetTypeVisitor.visit(type, new WriteBuilder(type, structWriterFactory));
}

private static class WriteBuilder extends ParquetTypeVisitor<ParquetValueWriter<?>> {
private final MessageType type;
private final StructWriterFactory structWriterFactory;

WriteBuilder(MessageType type) {
WriteBuilder(MessageType type, StructWriterFactory structWriterFactory) {
this.type = type;
this.structWriterFactory = structWriterFactory;
}

@Override
Expand All @@ -78,7 +85,7 @@ public ParquetValueWriter<?> struct(GroupType struct,
writers.add(ParquetValueWriters.option(fieldType, fieldD, fieldWriters.get(i)));
}

return new RecordWriter(writers);
return structWriterFactory.create(writers);
}

@Override
Expand Down Expand Up @@ -284,6 +291,11 @@ public void write(int repetitionLevel, byte[] value) {
}
}

public interface StructWriterFactory {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of adding this interface, what about adding WriteBuilder.createStructWriter to do this? That way, the Flink builder could just inherit from the generic builder and override that one method.

I think that would be cleaner because adding this interface requires also adding public methods to pass the factory. I'd rather not add those public methods if we can avoid it by adding a protected method and change the builder to a protected class.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I guess we will also need to mark the WriteBuilder from private to public because it will be accessed by flink classes from outside package.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It could be protected, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

protected is not enough for the outside flink package, the WriterBuilder is a static class in org.apache.iceberg.data.parquet , it must be public so that other packages could inherit it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You're right. The class should be public, but the method that will be overridden should be protected.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Em, I've implemented two versions. the first version provides a public interfaces with protected StructureWriterFactory and StructReaderFactory, the second version use the inherit WriterBuilder & ReaderBuilder #1125. For me , seems the first version looks much more concise. I plan to change to version#1 (with the public interface and protected methods ). Thanks.

@rdblue rdblue Jun 25, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you post the branch with the inheritance version? I'd like to see it to compare. I don't like the extra public classes and methods, and I think that inheritance would be a cleaner public API. I'm curious why you think the other approach looks more concise, though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The inheritance version is here.

I'm curious why you think the other approach looks more concise, though.

Because the inheritance version will need to expose the buildReader and buildWriter logic , for example this. If we change those logic, will also need to change in flink reader/writers. seems the same logic with the GenericParquetReader.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay, how about inheriting from the outer class, then? That way, the interface and the method that accepts the factory could be protected. Mainly, I don't think these should be public.


StructWriter<?> create(List<ParquetValueWriter<?>> writers);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I tried to mark this method to be protected but seems java8 don't allow to do that....

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interface methods are always public.

}

private static class RecordWriter extends StructWriter<Record> {
private RecordWriter(List<ParquetValueWriter<?>> writers) {
super(writers);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*
* 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
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.apache.iceberg.flink.data;

import java.util.List;
import org.apache.flink.types.Row;
import org.apache.iceberg.Schema;
import org.apache.iceberg.data.parquet.GenericParquetReaders;
import org.apache.iceberg.parquet.ParquetValueReader;
import org.apache.iceberg.parquet.ParquetValueReaders;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.types.Types;
import org.apache.parquet.schema.MessageType;
import org.apache.parquet.schema.Type;

public class FlinkParquetReaders {
private FlinkParquetReaders() {

}

public static ParquetValueReader<Row> buildReader(Schema expectedSchema,
MessageType fileSchema) {
return GenericParquetReaders.buildReader(expectedSchema, fileSchema, ImmutableMap.of(), RowReader::new);
}

static class RowReader extends ParquetValueReaders.StructReader<Row, Row> {
private final Types.StructType structType;

RowReader(List<Type> types, List<ParquetValueReader<?>> readers, Types.StructType struct) {
super(types, readers);
this.structType = struct;
}

@Override
protected Row newStructData(Row reuse) {
if (reuse != null) {
return reuse;
} else {
return new Row(structType.fields().size());
}
}

@Override
protected Object getField(Row row, int pos) {
return row.getField(pos);
}

@Override
protected Row buildStruct(Row row) {
return row;
}

@Override
protected void set(Row row, int pos, Object value) {
row.setField(pos, value);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* 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
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.apache.iceberg.flink.data;

import java.util.List;
import org.apache.flink.types.Row;
import org.apache.iceberg.data.parquet.GenericParquetWriter;
import org.apache.iceberg.parquet.ParquetValueWriter;
import org.apache.iceberg.parquet.ParquetValueWriters;
import org.apache.parquet.schema.MessageType;

public class FlinkParquetWriters {
private FlinkParquetWriters() {
}

@SuppressWarnings("unchecked")
public static <T> ParquetValueWriter<T> buildWriter(MessageType type) {
return GenericParquetWriter.buildWriter(type, RowWriter::new);
}

private static class RowWriter extends ParquetValueWriters.StructWriter<Row> {

private RowWriter(List<ParquetValueWriter<?>> writers) {
super(writers);
}

@Override
protected Object get(Row row, int index) {
return row.getField(index);
}
}
}
Loading