Add support for AVRO in Iceberg by ebyhr · Pull Request #12125 · trinodb/trino

ebyhr · 2022-04-25T12:42:26Z

Description

Add support for AVRO in Iceberg
Supersedes #4776

Documentation

(x) Sufficient documentation is included in this PR.

Release notes

(x) Release notes entries required with the following suggested text:

# Iceberg
* Add support for Avro file format. ({issue}`12125`)

jklamer · 2022-04-25T16:44:15Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergAvroDataConversion.java

This should always be string correct?

jklamer · 2022-04-25T16:46:26Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergAvroFileWriter.java

A more general question: Is there a big difference between the iceberg libraries for Avro / the Avro Libraries themselves? I know Ryan works on both so I know this works Im just curious if this is done to keep things in sync or ergonomics or some other reason?

jklamer · 2022-04-25T16:47:52Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergAvroDataConversion.java

Does the Iceberg Avro Library do away with the Utf8 class 😅

jklamer · 2022-04-25T16:52:02Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergAvroPageSource.java

this is neat

jklamer · 2022-04-25T17:12:25Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplit.java

General question: Why is this needed to be sent as part of the split if the schema is part of the file?

alexjo2144

Just an initial skim. Looks like Avro table stat's aren't working. Is that because Avro doesn't have them, or because the read path just isn't implemented for them yet?

alexjo2144 · 2022-04-28T17:33:14Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSourceProvider.java

I think these need to be in the else block?

Suggested change

rowIndexChannels.add(false);

columnNames.add(column.getName());

columnTypes.add(column.getType());

if (field == null) {

constantPopulatingPageSourceBuilder.addConstantColumn(nativeValueToBlock(column.getType(), null));

}

else {

constantPopulatingPageSourceBuilder.addDelegateColumn(avroSourceChannel);

}

avroSourceChannel++;

if (field == null) {

constantPopulatingPageSourceBuilder.addConstantColumn(nativeValueToBlock(column.getType(), null));

}

else {

rowIndexChannels.add(false);

columnNames.add(column.getName());

columnTypes.add(column.getType());

constantPopulatingPageSourceBuilder.addDelegateColumn(avroSourceChannel);

avroSourceChannel++;

}

ebyhr · 2022-04-30T08:14:21Z

Is that because Avro doesn't have them, or because the read path just isn't implemented for them yet?

Avro file/table doesn't support statistics as far as I know.

electrum

Apologies for the long review time. Let's get this in since it's needed for #13219

docs/src/main/sphinx/connector/iceberg.rst

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergAvroDataConversion.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergAvroFileWriter.java

electrum · 2022-07-19T17:48:20Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergAvroPageSource.java

Do we need multiple row ID columns? Or could this be a single index?

Since we're using the Iceberg Avro reader, I'm thinking we might get the row ID automatically by including the ROW_POSITION column in the schema.

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSourceProvider.java

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

ebyhr · 2022-07-21T12:36:06Z

Just rebased on upstream to resolve a logical conflict by 308e717

electrum · 2022-07-22T03:54:25Z

Looks like a real test failure:

2022-07-22 06:58:45 INFO: FAILURE     /    io.trino.tests.product.iceberg.TestIcebergSparkCompatibility.testSparkReadingTrinoCompressedData [AVRO, ZSTD] (Groups: profile_specific_tests, iceberg) took 0.6 seconds
2022-07-22 06:58:45 SEVERE: Failure cause:
java.lang.AssertionError: Expected callback to throw QueryExecutionException
	at io.trino.tempto.assertions.QueryAssert.assertQueryFailure(QueryAssert.java:107)
	at io.trino.tests.product.iceberg.TestIcebergSparkCompatibility.testSparkReadingTrinoCompressedData(TestIcebergSparkCompatibility.java:1238)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:645)
	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)

Co-Authored-By: Xingyuan Lin <xingyuan_lin@apple.com>

cla-bot bot added the cla-signed label Apr 25, 2022

github-actions bot added docs tests:hive labels Apr 25, 2022

findepi mentioned this pull request Apr 25, 2022

Add Avro support to Iceberg Connector #4776

Closed

jklamer reviewed Apr 25, 2022

View reviewed changes

ebyhr force-pushed the ebi/iceberg-avro branch 4 times, most recently from 7343096 to a5c6550 Compare April 28, 2022 03:08

ebyhr marked this pull request as ready for review April 28, 2022 03:22

ebyhr requested review from electrum and phd3 April 28, 2022 06:49

alexjo2144 reviewed Apr 28, 2022

View reviewed changes

ebyhr force-pushed the ebi/iceberg-avro branch from a5c6550 to b5cba79 Compare April 30, 2022 08:12

ebyhr mentioned this pull request May 11, 2022

Iceberg Connector #1324

Closed

93 tasks

ebyhr force-pushed the ebi/iceberg-avro branch from b5cba79 to 617b1dd Compare May 16, 2022 08:59

electrum approved these changes Jul 19, 2022

View reviewed changes

ebyhr force-pushed the ebi/iceberg-avro branch 2 times, most recently from cf3c508 to 53317f6 Compare July 21, 2022 12:34

ebyhr force-pushed the ebi/iceberg-avro branch from 53317f6 to c982721 Compare July 22, 2022 05:46

ebyhr and others added 2 commits July 25, 2022 09:15

Add support for AVRO in Iceberg

4251a39

Co-Authored-By: Xingyuan Lin <xingyuan_lin@apple.com>

empty

2a6639f

ebyhr force-pushed the ebi/iceberg-avro branch from cd89eb8 to 2a6639f Compare July 25, 2022 00:15

electrum merged commit 5438d7d into trinodb:master Jul 25, 2022

github-actions bot added this to the 392 milestone Jul 25, 2022

ebyhr deleted the ebi/iceberg-avro branch July 25, 2022 21:53

ebyhr mentioned this pull request Jul 25, 2022

Release notes for 392 #13320

Closed

colebow mentioned this pull request Jul 27, 2022

Add Trino 392 release notes #13342

Closed

-                    rowIndexChannels.add(false);
-                    columnNames.add(column.getName());
-                    columnTypes.add(column.getType());
-                    if (field == null) {
-                        constantPopulatingPageSourceBuilder.addConstantColumn(nativeValueToBlock(column.getType(), null));
-                    }
-                    else {
-                        constantPopulatingPageSourceBuilder.addDelegateColumn(avroSourceChannel);
-                    }
-                    avroSourceChannel++;
+                    if (field == null) {
+                        constantPopulatingPageSourceBuilder.addConstantColumn(nativeValueToBlock(column.getType(), null));
+                    }
+                    else {
+                        rowIndexChannels.add(false);
+                        columnNames.add(column.getName());
+                        columnTypes.add(column.getType());
+                        constantPopulatingPageSourceBuilder.addDelegateColumn(avroSourceChannel);
+                        avroSourceChannel++;
+                    }

Conversation

ebyhr commented Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Documentation

Release notes

Uh oh!

jklamer Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

jklamer Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

jklamer Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

jklamer Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

jklamer Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

alexjo2144 left a comment

Choose a reason for hiding this comment

Uh oh!

alexjo2144 Apr 28, 2022

Choose a reason for hiding this comment

Uh oh!

ebyhr commented Apr 30, 2022

Uh oh!

electrum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

electrum Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ebyhr commented Jul 21, 2022

Uh oh!

electrum commented Jul 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

ebyhr commented Apr 25, 2022 •

edited

Loading