Refactor the GenericOrcWriter by using OrcSchemaWithTypeVisitor#visit #1197

openinx · 2020-07-13T13:09:37Z

This PR did the refactor for GenericOrcWriter :

generate the orc writer by using OrcSchemaWithTypeVisitor#visit, so that we could abstract the common data type writers in a separated class named GenericOrcWriters.
In the future FlinkOrcWriter , it will share the common writers from GenericOrcWriters.

data/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriter.java

data/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriters.java

data/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriter.java

rdsr · 2020-07-14T00:04:14Z

data/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriter.java

-
-  public static OrcValueWriter<Record> buildWriter(TypeDescription fileSchema) {
-    return new GenericOrcWriter(fileSchema);
+  private final GenericOrcWriters.Converter converter;


Any benefit of using GenericOrcWriters.Converter<?>?

For me, it seems don't have much difference with a <?> or not, but I can changed to keep symmetry as we've discussed above.

data/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriter.java

rdsr · 2020-07-14T00:30:14Z

data/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriter.java

+    }
  }

  @SuppressWarnings("unchecked")


I guess this suppress unchecked can be removed once we paramterize the converter with a wildcard e.g converter -> converter<?>

+1

We should avoid using types without parameters.

Here I did not change to use OrcValueWriter<?> because if we do then we have the following to write child:

for (int c = 0; c < writers.size(); ++c) { OrcValueWriter<?> child = writers.get(c); child.write(row, value.get(c, child.getJavaClass()), output.cols[c]); }

The value is a StructLike and the get in StructLike is <T> T get(int pos, Class<T> javaClass), while child.getJavaClass is a class like OrcValueWriter<?>.class, it will throw the compile error:

Incompatible types. Required capture of ? but 'get' was inferred to T: no instance(s) of of type variables(s) exist so that capture of ? conforms to capture of ?

data/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriters.java

openinx · 2020-07-14T04:19:45Z

@rdsr @shardulm94 Mind to take another look ? I've updated the pull request, Thanks.

shardulm94 · 2020-07-14T05:19:48Z

orc/src/main/java/org/apache/iceberg/orc/OrcFileAppender.java

-  private static <D> OrcValueWriter<D> newOrcValueWriter(
-      TypeDescription schema, Function<TypeDescription, OrcValueWriter<?>> createWriterFunc) {
-    return (OrcValueWriter<D>) createWriterFunc.apply(schema);
+  private static <D> OrcRowWriter<D> newOrcValueWriter(


Nit: Change method name to reflect class name change

Thanks for your reminding, Good point.

data/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriter.java

openinx · 2020-07-15T10:08:42Z

Ping @rdsr @shardulm94 @rdblue , any other concern ? Thanks.

rdblue · 2020-07-15T20:41:03Z

data/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriters.java

+    }
+  }
+
+  private static class MapWriter implements OrcValueWriter<Map> {


Missing parameter types.

rdblue · 2020-07-15T20:41:18Z

data/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriters.java

+    }
+  }
+
+  private static class ListWriter implements OrcValueWriter<List> {


Missing parameter type.

I did not provide the explicit parameter type here, because getJavaClass() will need to return a List class with generic type, while Java don't support this now. Pls see here

We still need to add the parameterized types.

The FAQ entry you pointed to explains why there is no class literal, like List<String>.class. All variants of List use List.class because there is only one concrete type at runtime. But we still want to use type parameters to be explicit about what is passed around.

This class handles lists of some type, T. The class should be parameterized by T so that we can use type-safe operations to pass around T instances. The wrapped value writer should be OrcValueWriter<T> elementWriter. By doing this, the implementation of nonNullWrite will get a List<T> and will be able to pass those values to the elementWriter without casting.

orc/src/main/java/org/apache/iceberg/orc/OrcFileAppender.java

rdblue · 2020-07-15T20:45:36Z

spark/src/main/java/org/apache/iceberg/spark/data/SparkOrcWriter.java

+     * @param rowId  the row in the ColumnVector
     * @param column either the column number or element number
-     * @param data either an InternalRow or ArrayData
+     * @param data   either an InternalRow or ArrayData


Nit: unnecessary whitespace changes.

OK, I guess I did a code format before.

openinx · 2020-07-21T01:55:14Z

@rdblue @rdsr I think this patch is ready to merge now, please merge this if no other concern.

rdblue · 2020-07-21T16:33:54Z

data/src/test/java/org/apache/iceberg/data/TestMetricsRowGroupFilterTypes.java

    try (FileAppender<Record> appender = ORC.write(outFile)
        .schema(FILE_SCHEMA)
-        .createWriterFunc(GenericOrcWriter::buildWriter)
+        .createWriterFunc(typeDesc -> GenericOrcWriter.buildWriter(FILE_SCHEMA, typeDesc))


Since the schema is passed to the write builder, what about adding a createWriterFunc method that accepts BiFunction<Schema, TypeDescription>? Then this wouldn't need to change.

We do this in Avro: https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/avro/Avro.java#L199-L203

That would cut down on the number of files that need to change in this PR.

OK, that sounds good to me.

Would it make more sense to add createWriterFunc method that accepts BiFunction<Schema, TypeDescription> instead of replacing the existing one? Replacing the existing createWriterFunc causes changes in the files?

TestSparkOrcReadMetadataColumns.java TestSparkOrcReader.java TestOrcWrite.java SparkAppenderFactory.java

rdblue · 2020-07-21T16:36:06Z

orc/src/main/java/org/apache/iceberg/orc/OrcValueWriter.java

- */
 public interface OrcValueWriter<T> {

+  Class<T> getJavaClass();


What will this be used for? I don't see anything calling it in this commit.

It's used for reading the field from Record and casting the value to target class , see here: https://github.com/apache/iceberg/pull/1197/files#diff-69c0f1e45966d2eb49a315fe32734cf5R125.

openinx

Addressed all the comments (Include missing parameter type, Function -> BiFunction).

openinx · 2020-07-22T03:22:47Z

orc/src/main/java/org/apache/iceberg/orc/OrcValueWriter.java

- */
 public interface OrcValueWriter<T> {

+  Class<T> getJavaClass();


It's used for reading the field from Record and casting the value to target class , see here: https://github.com/apache/iceberg/pull/1197/files#diff-69c0f1e45966d2eb49a315fe32734cf5R125.

openinx · 2020-07-22T03:25:48Z

data/src/test/java/org/apache/iceberg/data/TestMetricsRowGroupFilterTypes.java

    try (FileAppender<Record> appender = ORC.write(outFile)
        .schema(FILE_SCHEMA)
-        .createWriterFunc(GenericOrcWriter::buildWriter)
+        .createWriterFunc(typeDesc -> GenericOrcWriter.buildWriter(FILE_SCHEMA, typeDesc))


OK, that sounds good to me.

… OrcValueWriter.

…ender#newOrcRowWriter.

…ases.

rdsr · 2020-07-22T18:16:31Z

Thanks @openinx ! I will have another look, today.

data/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriters.java

orc/src/main/java/org/apache/iceberg/orc/ORC.java

spark/src/main/java/org/apache/iceberg/spark/data/SparkOrcWriter.java

orc/src/main/java/org/apache/iceberg/orc/OrcRowWriter.java

rdsr

The changes look great to me! We just need to address very minor comments and a question on replacing vs adding a BiFunction.

openinx

About this issue #1197 (comment), I think we'd better not to introduce another createWriterFunc method here, sir. That makes more confuse and the OrcFileAppender need to choose the non-null function to create the OrcRowWriter. Now, for both Function argument or BiFunction argument, we need to change few files, then we just choose one.

I will push the next patch to address other comments, Thanks for the patient reviewing.

spark/src/main/java/org/apache/iceberg/spark/data/SparkOrcWriter.java

openinx · 2020-07-26T13:02:59Z

Ping @rdblue @rdsr , Mind to take a look again and help to merge this PR ? Thanks.

rdsr · 2020-07-26T22:39:07Z

@openinx
I felt adding a BiFunction was better than replacing it. And regarding

That makes more confuse and the OrcFileAppender need to choose the non-null function to create the OrcRowWriter

Avro handles that well. See org.apache.iceberg.avro.Avro.ReadBuilder#build
But I'm ok with the patch in its current form. Since @rdblue initiated this comment. I'll wait for his final approval before merging.

openinx · 2020-07-27T01:59:40Z

@rdsr I've refactored the SparkOrcWrier by using OrcSchemaWithTypeVisitor in here, we can see that the constructor of SparkOrcWriter will also need the two arguments: iceberg schema and TypeDescription. So actually, although we could add a Function in here, but we actually won't use it in the newly refactored spark orc writer, that's why I say we don't need to introduce the redundant Function method.

rdsr · 2020-07-27T03:23:23Z

@rdsr I've refactored the SparkOrcWrier by using OrcSchemaWithTypeVisitor in here, we can see that the constructor of SparkOrcWriter will also need the two arguments: iceberg schema and TypeDescription. So actually, although we could add a Function in here, but we actually won't use it in the newly refactored spark orc writer, that's why I say we don't need to introduce the redundant Function method.

@openinx That makes sense to me. Thanks!

openinx · 2020-07-27T07:17:19Z

Thanks for the confirmation, so if no other concern, please help to merge this PR so that we could move the following flink ORC reader writer work forward. Thanks in advance.

rdsr

LGTM! Giving it a day for others to have a look before we merge. Thanks @openinx !

simonsssu · 2020-07-29T06:22:00Z

Can we abstract a BaseOrcWriter in the future ? Then make GenericOrcWriter, FlinkOrcWriter, SparkOrcWriter extends it.

openinx · 2020-07-29T13:59:34Z

@simon0806 we won't need to abstract the BaseOrcWriter again in this patch because we have discussed to share the common data type writers in the GenericOrcWriter, FlinkOrcWriter, SparkOrcWriter. That's helpful to decouple the codes for different compute engine. Thanks.

BTW, ping @rdblue to merge this patch, Thanks.

rdblue · 2020-07-29T20:58:58Z

Looks good. Thanks, @openinx!

…e#1197)

rdblue requested a review from rdsr July 13, 2020 17:35

shardulm94 reviewed Jul 13, 2020

View reviewed changes

rdsr reviewed Jul 14, 2020

View reviewed changes

data/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriter.java Outdated Show resolved Hide resolved

rdsr reviewed Jul 14, 2020

View reviewed changes

shardulm94 reviewed Jul 14, 2020

View reviewed changes

openinx commented Jul 14, 2020

View reviewed changes

data/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriter.java Show resolved Hide resolved

openinx mentioned this pull request Jul 15, 2020

Flink: Add wrapper to adapt Row to StructLike #1175

Merged

rdblue reviewed Jul 15, 2020

View reviewed changes

orc/src/main/java/org/apache/iceberg/orc/OrcFileAppender.java Show resolved Hide resolved

rdblue reviewed Jul 15, 2020

View reviewed changes

rdblue reviewed Jul 21, 2020

View reviewed changes

rdblue mentioned this pull request Jul 21, 2020

Flink: Add Orc value reader, writer implementations #1158

Closed

openinx commented Jul 22, 2020

View reviewed changes

openinx added 11 commits July 22, 2020 11:59

Refactor the GenericOrcWriter by using OrcSchemaWithTypeVisitor#visit

b4164d9

Refactor the OrcValueWriter to OrcRowWriter and refactor converter to…

e21eade

… OrcValueWriter.

Remove the OrcValue from all the writer names.

e88f678

Addressing serveral minor issues

afe95c7

Fixed broken unit tests.

cd5580b

Minor fix: rename the OrcFileAppender#newOrcValueWriter to OrcFileApp…

e29467a

…ender#newOrcRowWriter.

align the class name of writers and use the iceberg types in switch-c…

4fce2d0

…ases.

Fix checkstyle

c6db227

Addressing the comment.

a19855f

return the OrcValueWriter with a type.

c147747

typo

6b6bb4e

openinx added 3 commits July 22, 2020 11:59

Provide the parameter type for ListWriter and MapWriter

7a021e3

User the BiFunction instead of Function to reduce code changes.

c37a207

Rebase and fix the broken unit tests.

d5fcd98

rdsr reviewed Jul 23, 2020

View reviewed changes

data/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriters.java Outdated Show resolved Hide resolved

rdsr reviewed Jul 23, 2020

View reviewed changes

orc/src/main/java/org/apache/iceberg/orc/ORC.java Outdated Show resolved Hide resolved

rdsr reviewed Jul 23, 2020

View reviewed changes

spark/src/main/java/org/apache/iceberg/spark/data/SparkOrcWriter.java Show resolved Hide resolved

orc/src/main/java/org/apache/iceberg/orc/OrcRowWriter.java Show resolved Hide resolved

rdsr reviewed Jul 23, 2020

View reviewed changes

openinx mentioned this pull request Jul 23, 2020

Refactor the SparkOrcWriter by using OrcSchemaWithTypeVisitor for sharing writers from GenericOrcWriters #1236

Closed

Addressing the comments from rdsr

e820e89

openinx commented Jul 23, 2020

View reviewed changes

spark/src/main/java/org/apache/iceberg/spark/data/SparkOrcWriter.java Show resolved Hide resolved

openinx mentioned this pull request Jul 24, 2020

Refactor the SparkOrcWriter by using OrcSchemaWithTypeVisitor#visit #1238

Merged

rdsr approved these changes Jul 28, 2020

View reviewed changes

rdblue merged commit d2ceb5b into apache:master Jul 29, 2020

openinx deleted the geneirc-orc-refactor branch July 30, 2020 00:24

openinx mentioned this pull request Jul 30, 2020

Flink: Add Orc value reader, writer implementations #1255

Merged

cmathiesen pushed a commit to ExpediaGroup/iceberg that referenced this pull request Aug 19, 2020

ORC: Refactor GenericOrcWriter to use OrcSchemaWithTypeVisitor (apach…

ba64a61

…e#1197)

Refactor the GenericOrcWriter by using OrcSchemaWithTypeVisitor#visit #1197

Refactor the GenericOrcWriter by using OrcSchemaWithTypeVisitor#visit #1197

Uh oh!

Conversation

openinx commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openinx Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

openinx commented Jul 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openinx commented Jul 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rdblue Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openinx commented Jul 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openinx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

openinx commented Jul 13, 2020 •

edited

Loading

openinx Jul 21, 2020 •

edited

Loading

rdblue Jul 21, 2020 •

edited

Loading

rdblue Jul 15, 2020 •

edited

Loading

rdsr left a comment •

edited

Loading

rdsr commented Jul 26, 2020 •

edited

Loading

rdsr left a comment •

edited

Loading