Disable support for snapshot id within the Iceberg table name #13414

findinpath · 2022-07-29T12:50:30Z

Description

Disable the possibility of specifying the snapshot id
within the Iceberg table name.

With the introduction of the syntax

table1 FOR VERSION AS OF 123

there is no more need of supporting the
deprecated syntax:

table1@123

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

Other.

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?

Use table1 FOR VERSION AS OF 123 instead of table1@123 for retrieving the snapshot 123 of table.

Related issues, pull requests, and links

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
* Disable support for specifying the snapshot id within the Iceberg table name

findinpath · 2022-07-29T12:51:15Z

@findepi @phd3 @electrum PTAL on this PR

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

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

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

alexjo2144 · 2022-07-29T13:48:07Z

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

                return Optional.of(new SnapshotsTable(systemTableName, typeManager, table));
            case PARTITIONS:
-                return Optional.of(new PartitionTable(systemTableName, typeManager, table, getSnapshotId(table, name.getSnapshotId(), isAllowLegacySnapshotSyntax(session))));
+                return Optional.of(new PartitionTable(systemTableName, typeManager, table, Optional.empty()));


This looks like a regression to me, does the AS OF syntax work as a replacement for select * from abc$manifests@456?

I have a PR for supporting time travel queries on system tables but it is unfortunately not ready yet.

#12737

Probably it is best to introduce the ability to perform time travel queries on system tables before continuing with this PR.

As shown in the PRs:

Support snapshot queries on Iceberg system tables - IcebergSystemTableHandle is a ConnectorTableHandle with custom instanceof handling in IcebergMetadata #13497

Support snapshot queries on Iceberg system tables - custom IcebergSystemTableHandle extends SystemTableHandle #13854

time travel on system tables incurs added complexity to the Iceberg connector code.

Without concrete demand from the community for time travel functionality on system tables, we leave the above mentioned PRs on hold and go further with disabling support for snapshot id within the Iceberg table name.

This looks like a regression to me

correct

does the AS OF syntax work as a replacement for select * from abc$manifests@456?

no

The reason we're not pursing the above linked PRs is because we believe there is little community interest in these.
If there isn't general demand on time travel on system tables, we should be OK with carrying out removal of this (already deprecated) functionality.

findinpath · 2022-09-06T15:56:48Z

In the context of deprecating the tableName@snapshotId syntax, maybe it is worth also rewriting io.trino.plugin.iceberg.IcebergTableHandle#toString to use tableName FOR VERSION AS OF snapshotId syntax.

findepi · 2022-09-07T09:55:05Z

@findinpath there is a test failure. is it related?

findepi · 2022-09-07T09:56:07Z

In the context of deprecating the tableName@snapshotId syntax, maybe it is worth also rewriting io.trino.plugin.iceberg.IcebergTableHandle#toString to use tableName FOR VERSION AS OF snapshotId syntax.

This is used in EXPLAIN.
Let's keep it terse (no change), since this will always be there, for every SELECT, not only for time travel queries.

findinpath · 2022-09-07T12:59:28Z

@findepi failure of the tests was related to my changes. I fixed the failing test failure and rebased on master due to conflicts which appeared in the meantime.

findepi · 2022-09-09T14:54:13Z

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

-            long snapshotId = endVersion.map(connectorTableVersion -> getSnapshotIdFromVersion(table, connectorTableVersion))
-                    .orElseGet(() -> resolveSnapshotId(table, name.getSnapshotId().get(), isAllowLegacySnapshotSyntax(session)));
+        if (endVersion.isPresent()) {
+            long snapshotId = endVersion.map(connectorTableVersion -> getSnapshotIdFromVersion(table, connectorTableVersion)).get();


Suggested change

long snapshotId = endVersion.map(connectorTableVersion -> getSnapshotIdFromVersion(table, connectorTableVersion)).get();

long snapshotId = getSnapshotIdFromVersion(table, endVersion.get());

findepi · 2022-09-09T14:54:57Z

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

-    }
-
-    private long resolveSnapshotId(Table table, long id, boolean allowLegacySnapshotSyntax)
+    private Optional<Long> getSnapshotId(Table table)


getCurrentSnapshotId

Disable the possibility of specifying the snapshot id within the Iceberg table name. With the introduction of the syntax ``` table1 FOR VERSION AS OF 123 ``` there is no more need of supporting the deprecated syntax: ``` table1@123 ```

findepi · 2022-09-12T07:51:07Z

No release notes since the functionality was already deprecated.

cla-bot bot added the cla-signed label Jul 29, 2022

findinpath requested review from findepi, phd3 and electrum and removed request for findepi July 29, 2022 12:51

findepi requested a review from alexjo2144 July 29, 2022 13:20

findepi reviewed Jul 29, 2022

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Outdated Show resolved Hide resolved

alexjo2144 reviewed Jul 29, 2022

View reviewed changes

findinpath force-pushed the iceberg-disable-usage-of-snapshotid-in-table-name branch from 80a5cca to 89b53dd Compare July 29, 2022 19:45

findinpath marked this pull request as draft July 29, 2022 20:00

findinpath force-pushed the iceberg-disable-usage-of-snapshotid-in-table-name branch from 89b53dd to c31a47e Compare September 6, 2022 14:46

findinpath marked this pull request as ready for review September 6, 2022 14:47

findinpath requested review from findepi and alexjo2144 September 6, 2022 14:48

findinpath force-pushed the iceberg-disable-usage-of-snapshotid-in-table-name branch from c31a47e to 2c58a97 Compare September 6, 2022 16:00

alexjo2144 approved these changes Sep 6, 2022

View reviewed changes

findinpath force-pushed the iceberg-disable-usage-of-snapshotid-in-table-name branch from 2c58a97 to 3300dee Compare September 7, 2022 13:02

findinpath self-assigned this Sep 7, 2022

findinpath force-pushed the iceberg-disable-usage-of-snapshotid-in-table-name branch 2 times, most recently from 2c07399 to 3a85f75 Compare September 9, 2022 14:06

findepi approved these changes Sep 9, 2022

View reviewed changes

findinpath force-pushed the iceberg-disable-usage-of-snapshotid-in-table-name branch from 3a85f75 to 51187fb Compare September 12, 2022 04:18

findinpath requested a review from findepi September 12, 2022 04:22

findepi merged commit 7360b11 into trinodb:master Sep 12, 2022

findepi added the no-release-notes This pull request does not require release notes entry label Sep 12, 2022

github-actions bot added this to the 396 milestone Sep 12, 2022

colebow mentioned this pull request Sep 13, 2022

Add Trino 396 release notes #14109

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable support for snapshot id within the Iceberg table name #13414

Disable support for snapshot id within the Iceberg table name #13414

findinpath commented Jul 29, 2022

findinpath commented Jul 29, 2022

alexjo2144 Jul 29, 2022

findinpath Jul 29, 2022 •

edited

Loading

findinpath Jul 29, 2022

findinpath Sep 6, 2022

findepi Sep 7, 2022

findinpath commented Sep 6, 2022

findepi commented Sep 7, 2022

findepi commented Sep 7, 2022

findinpath commented Sep 7, 2022

findepi Sep 9, 2022

findepi Sep 9, 2022

findepi commented Sep 12, 2022

	long snapshotId = endVersion.map(connectorTableVersion -> getSnapshotIdFromVersion(table, connectorTableVersion)).get();
	long snapshotId = getSnapshotIdFromVersion(table, endVersion.get());

Disable support for snapshot id within the Iceberg table name #13414

Disable support for snapshot id within the Iceberg table name #13414

Conversation

findinpath commented Jul 29, 2022

Description

Related issues, pull requests, and links

Documentation

Release notes

findinpath commented Jul 29, 2022

alexjo2144 Jul 29, 2022

Choose a reason for hiding this comment

findinpath Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

findinpath Jul 29, 2022

Choose a reason for hiding this comment

findinpath Sep 6, 2022

Choose a reason for hiding this comment

findepi Sep 7, 2022

Choose a reason for hiding this comment

findinpath commented Sep 6, 2022

findepi commented Sep 7, 2022

findepi commented Sep 7, 2022

findinpath commented Sep 7, 2022

findepi Sep 9, 2022

Choose a reason for hiding this comment

findepi Sep 9, 2022

Choose a reason for hiding this comment

findepi commented Sep 12, 2022

findinpath Jul 29, 2022 •

edited

Loading