Iceberg: use native implementation to obtain snapshot schema #13614

xiacongling · 2022-08-11T09:49:23Z

Description

Is this change a fix, improvement, new feature, refactoring, or other?

a fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Iceberg Connector

How would you describe this change to a non-technical end user or system administrator?

Uss native implementation to obtain snapshot schema when executing Iceberg time travel queries. Time travel queries on a table created with Iceberg<0.12 will fail without this fix.

Related issues, pull requests, and links

related to #12743 #12786
fixes #13613

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

() No release notes entries required.
(x) Release notes entries required with the following suggested text

# Iceberg
* Fix failure when executing time travel queries on a table created with Iceberg<0.12. ({issue}`13613`)

cla-bot · 2022-08-11T09:49:26Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

findepi

@xiacongling would you be able to add a regression product test for this case?

see TestIcebergSparkCompatibility for existing tests

xiacongling · 2022-08-11T12:43:48Z

@findepi sure

cla-bot · 2022-08-12T04:53:14Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

xiacongling · 2022-08-12T05:09:23Z

Spend some time with the testing framework and finally get it work. A case is added in SuiteCompatibility for the reason that Iceberg metafile is changing over times, we must make sure tables created by an early version of Trino can be queried normally in newer versions.

Early versions (<= 364) of Trino use iceberg v0.11 which did not support time traveling. The snapshot did not have a field named schema-id in its metadata JSON file. After updated to Trino 392, it will fail to query such tables with the following exception message:

io.trino.tempto.query.QueryExecutionException: java.sql.SQLException: Query failed (#20220812_044558_00002_36g47): Cannot invoke "org.apache.iceberg.Schema.asStruct()" because "schema" is null

xiacongling · 2022-08-12T05:19:30Z

@findepi Could you review this PR?

...-launcher/src/main/java/io/trino/tests/product/launcher/suite/suites/SuiteCompatibility.java

...ests/src/main/java/io/trino/tests/product/iceberg/TestIcebergFormatVersionCompatibility.java

cla-bot · 2022-08-12T11:47:17Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

findinpath · 2022-08-12T12:31:43Z

@xiacongling please squash the commits and follow the
guideline for the commit messages:

https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

alexjo2144 · 2022-08-12T13:00:33Z

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

@@ -326,7 +327,7 @@ public IcebergTableHandle getTableHandle(
            long snapshotId = endVersion.map(connectorTableVersion -> getSnapshotIdFromVersion(table, connectorTableVersion))
                    .orElseGet(() -> resolveSnapshotId(table, name.getSnapshotId().get(), isAllowLegacySnapshotSyntax(session)));
            tableSnapshotId = Optional.of(snapshotId);
-            tableSchema = table.schemas().get(table.snapshot(snapshotId).schemaId());
+            tableSchema = schemaFor(table, snapshotId);


For reference, here's what that method does:

public static Schema schemaFor(Table table, long snapshotId) { Snapshot snapshot = table.snapshot(snapshotId); Preconditions.checkArgument(snapshot != null, "Cannot find snapshot with ID %s", snapshotId); Integer schemaId = snapshot.schemaId(); // schemaId could be null, if snapshot was created before Iceberg added schema id to snapshot if (schemaId != null) { Schema schema = table.schemas().get(schemaId); Preconditions.checkState(schema != null, "Cannot find schema with schema id %s", schemaId); return schema; } // TODO: recover the schema by reading previous metadata files return table.schema(); }

cla-bot · 2022-08-12T13:42:35Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

xiacongling · 2022-08-12T13:44:23Z

@findinpath done

...ests/src/main/java/io/trino/tests/product/iceberg/TestIcebergFormatVersionCompatibility.java

cla-bot · 2022-08-15T02:13:22Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2022-08-16T13:59:24Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Iceberg snapshot definition in table metadata file does not have a schema-id until Iceberg version 0.12. Time travel queries on a table created by early versions of Trino (<=364) will fail without this fix.

cla-bot · 2022-08-17T03:20:42Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

ebyhr · 2022-08-17T04:58:48Z

I confirmed @xiacongling already submit CLA last week.
cc: @martint

martint · 2022-08-18T21:37:58Z

@cla-bot check

cla-bot · 2022-08-18T21:38:01Z

The cla-bot has been summoned, and re-checked this pull request!

ebyhr · 2022-08-18T22:28:59Z

Merged, thanks!

findepi approved these changes Aug 11, 2022

View reviewed changes

github-actions bot added the tests:hive label Aug 12, 2022

findepi reviewed Aug 12, 2022

View reviewed changes

xiacongling force-pushed the time-travel-query-bugfix branch from 55e9583 to 0c85b4d Compare August 12, 2022 11:47

findepi requested review from hashhar, findinpath and alexjo2144 August 12, 2022 12:23

alexjo2144 reviewed Aug 12, 2022

View reviewed changes

xiacongling force-pushed the time-travel-query-bugfix branch from 0c85b4d to 7ba88af Compare August 12, 2022 13:42

homar approved these changes Aug 12, 2022

View reviewed changes

...ests/src/main/java/io/trino/tests/product/iceberg/TestIcebergFormatVersionCompatibility.java Outdated Show resolved Hide resolved

xiacongling force-pushed the time-travel-query-bugfix branch from 7ba88af to 66ae1dc Compare August 15, 2022 02:13

findinpath approved these changes Aug 16, 2022

View reviewed changes

xiacongling force-pushed the time-travel-query-bugfix branch from 66ae1dc to 45387dc Compare August 16, 2022 13:59

Use Iceberg native implementation to obtain snapshot schema

f67c2f1

Iceberg snapshot definition in table metadata file does not have a schema-id until Iceberg version 0.12. Time travel queries on a table created by early versions of Trino (<=364) will fail without this fix.

ebyhr force-pushed the time-travel-query-bugfix branch from 45387dc to f67c2f1 Compare August 17, 2022 03:20

cla-bot bot added the cla-signed label Aug 18, 2022

ebyhr merged commit adcfbe9 into trinodb:master Aug 18, 2022

ebyhr mentioned this pull request Aug 18, 2022

Release notes for 394 #13723

Closed

github-actions bot added this to the 394 milestone Aug 18, 2022

colebow mentioned this pull request Aug 24, 2022

Add Trino 394 release notes #13813

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iceberg: use native implementation to obtain snapshot schema #13614

Iceberg: use native implementation to obtain snapshot schema #13614

xiacongling commented Aug 11, 2022 •

edited

Loading

cla-bot bot commented Aug 11, 2022

findepi left a comment

xiacongling commented Aug 11, 2022

cla-bot bot commented Aug 12, 2022

xiacongling commented Aug 12, 2022 •

edited

Loading

xiacongling commented Aug 12, 2022

cla-bot bot commented Aug 12, 2022

findinpath commented Aug 12, 2022

alexjo2144 Aug 12, 2022

cla-bot bot commented Aug 12, 2022

xiacongling commented Aug 12, 2022

cla-bot bot commented Aug 15, 2022

cla-bot bot commented Aug 16, 2022

cla-bot bot commented Aug 17, 2022

ebyhr commented Aug 17, 2022

martint commented Aug 18, 2022

cla-bot bot commented Aug 18, 2022

ebyhr commented Aug 18, 2022

Iceberg: use native implementation to obtain snapshot schema #13614

Iceberg: use native implementation to obtain snapshot schema #13614

Conversation

xiacongling commented Aug 11, 2022 • edited Loading

Description

Related issues, pull requests, and links

Documentation

Release notes

cla-bot bot commented Aug 11, 2022

findepi left a comment

Choose a reason for hiding this comment

xiacongling commented Aug 11, 2022

cla-bot bot commented Aug 12, 2022

xiacongling commented Aug 12, 2022 • edited Loading

xiacongling commented Aug 12, 2022

cla-bot bot commented Aug 12, 2022

findinpath commented Aug 12, 2022

alexjo2144 Aug 12, 2022

Choose a reason for hiding this comment

cla-bot bot commented Aug 12, 2022

xiacongling commented Aug 12, 2022

cla-bot bot commented Aug 15, 2022

cla-bot bot commented Aug 16, 2022

cla-bot bot commented Aug 17, 2022

ebyhr commented Aug 17, 2022

martint commented Aug 18, 2022

cla-bot bot commented Aug 18, 2022

ebyhr commented Aug 18, 2022

xiacongling commented Aug 11, 2022 •

edited

Loading

xiacongling commented Aug 12, 2022 •

edited

Loading