[Iceberg] Add Iceberg metadata table $ref by agrawalreetika · Pull Request #23503 · prestodb/presto

agrawalreetika · 2024-08-23T06:46:15Z

Description

Add Iceberg metadata table $ref

Motivation and Context

Add Iceberg metadata table $ref
provides details around tags & branches on the Iceberg table - https://iceberg.apache.org/docs/nightly/branching

Impact

Iceberg Connector
Provides details around tags & branches on the Iceberg table - https://iceberg.apache.org/docs/nightly/branching

Test Plan

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

== RELEASE NOTES ==

Iceberg Connector Changes
* Add Iceberg metadata table $ref :pr:`23503`

steveburnett

Thanks for the doc! A single tiny nit about punctuation, everything else looks good.

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

tdcmeehan

Nice change! Just a couple of nits.

tdcmeehan · 2024-08-23T16:13:02Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/RefsTable.java

+        return new FixedPageSource(buildPages(tableMetadata, icebergTable));
+    }
+
+    private static boolean checkNonNull(Object object, PageListBuilder pagesBuilder)


It seems to only be used for bigint values, so I'd just call this something like appendLongValue and append null if it's null, otherwise append the long value.

Actaully I just noticed value.minSnapshotsToKeep() is an Integer. I can add it as appendLongValue itself and use appendLongValue method. Or I can keep this method like earlier and append it with pagesBuilder.appendInteger LMK?

I'm just wondering if we can encapsulate the setting of values, either null or bigint, into one method. So perhaps just appendValue?

tdcmeehan · 2024-08-23T16:13:32Z

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

+
+.. code-block:: text
+
+      name |  type  |     snapshot_id     | max_reference_age_in_ms | min_snapshots_to_keep | max_snapshot_age_in_ms


It would be nice to include a non-main branch as well.

hantangwangd

Thanks for adding this metadata table, the change looks good to me. Should we add a test case to show that there will always be a main branch by default?

agrawalreetika · 2024-08-26T09:11:05Z

Thanks for adding this metadata table, the change looks good to me. Should we add a test case to show that there will always be a main branch by default?

Thanks for your review @hantangwangd , I have added this. please review.

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build, looks good. Thanks!

ZacBlanco · 2024-08-26T17:56:21Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/RefsTable.java

+    @Override
+    public Distribution getDistribution()
+    {
+        return Distribution.SINGLE_COORDINATOR;


nit: static import

ZacBlanco · 2024-08-26T18:08:18Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/RefsTable.java

+        icebergTable.refs().forEach((key, value) -> {
+            pagesBuilder.beginRow();
+            pagesBuilder.appendVarchar(key);
+            pagesBuilder.appendVarchar(value.isTag() ? "TAG" : "BRANCH");


What do you think about doing String.valueOf(value.type())? This way it handles updates to the SnapshotRefType enum.

ZacBlanco · 2024-08-26T18:18:54Z

presto-iceberg/src/test/java/com/facebook/presto/iceberg/TestIcebergSystemTables.java

+
+        // Check main branch entry
+        assertQuery("SELECT count(*) FROM test_schema.\"test_table$refs\"", "VALUES 1");
+        assertQuery("SELECT name FROM test_schema.\"test_table$refs\"", "VALUES 'main'");


I think it would also be prudent to add assertions on values in the row of the table matching the SnapshotRef object.

You could add or refactor the loadTable method from IcebergDistributedTestBase to get a reference to the Iceberg table, then re-create the values and run assertQuery

Added tests inIcebergDistributedTestBase which I could reuse in my subsequent PR around adding support for querying tags & branch. Please review

hantangwangd

Thanks for the added test case, overall looks good to me, one little nit.

hantangwangd · 2024-08-27T15:51:36Z

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java

+        assertEquals(icebergTable.refs().size(), 2);
+        icebergTable.manageSnapshots().createTag("testTag", icebergTable.currentSnapshot().snapshotId()).commit();
+
+        assertEquals(icebergTable.refs().size(), 3);
+        assertQuery("SELECT count(*) FROM \"test_table_references$refs\"", "VALUES 3");


Maybe we can add some more detailed assertions like follows:

assertQuery("SELECT * from \"test_table_references$refs\" where name = 'main' and type = 'BRANCH'", format("VALUES('%s', '%s', %s, %s, %s, %s)", "main", "BRANCH", icebergTable.refs().get("main").snapshotId(), icebergTable.refs().get("main").maxRefAgeMs(), icebergTable.refs().get("main").minSnapshotsToKeep(), icebergTable.refs().get("main").maxSnapshotAgeMs())); assertQuery("SELECT * from \"test_table_references$refs\" where type = 'TAG'", format("VALUES('%s', '%s', %s, %s, %s, %s)", "testTag", "TAG", icebergTable.refs().get("testTag").snapshotId(), icebergTable.refs().get("testTag").maxRefAgeMs(), icebergTable.refs().get("testTag").minSnapshotsToKeep(), icebergTable.refs().get("testTag").maxSnapshotAgeMs()));

Sure, I have added detailed assertions for testTag & testBranch instead of the main branch. Please LMK if that's fine

agrawalreetika requested review from a team, ZacBlanco, elharo, hantangwangd and steveburnett as code owners August 23, 2024 06:46

agrawalreetika requested a review from presto-oss August 23, 2024 06:46

agrawalreetika self-assigned this Aug 23, 2024

steveburnett requested changes Aug 23, 2024

View reviewed changes

presto-docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved

tdcmeehan reviewed Aug 23, 2024

View reviewed changes

agrawalreetika force-pushed the iceberg-ref-table branch from a0c4498 to 15f3194 Compare August 24, 2024 07:35

hantangwangd reviewed Aug 26, 2024

View reviewed changes

agrawalreetika force-pushed the iceberg-ref-table branch from 15f3194 to 1a0eea7 Compare August 26, 2024 09:10

steveburnett previously approved these changes Aug 26, 2024

View reviewed changes

agrawalreetika dismissed steveburnett’s stale review via 54eb31b August 26, 2024 17:52

agrawalreetika force-pushed the iceberg-ref-table branch from 1a0eea7 to 54eb31b Compare August 26, 2024 17:52

ZacBlanco requested changes Aug 26, 2024

View reviewed changes

agrawalreetika force-pushed the iceberg-ref-table branch from 54eb31b to e5d2c74 Compare August 27, 2024 06:11

ZacBlanco previously approved these changes Aug 27, 2024

View reviewed changes

hantangwangd reviewed Aug 27, 2024

View reviewed changes

Add Iceberg metadata table $refs

9fac986

agrawalreetika dismissed ZacBlanco’s stale review via 9fac986 August 27, 2024 18:35

agrawalreetika force-pushed the iceberg-ref-table branch from e5d2c74 to 9fac986 Compare August 27, 2024 18:35

ZacBlanco approved these changes Aug 27, 2024

View reviewed changes

hantangwangd approved these changes Aug 27, 2024

View reviewed changes

tdcmeehan approved these changes Aug 28, 2024

View reviewed changes

agrawalreetika merged commit 225b206 into prestodb:master Aug 28, 2024

agrawalreetika deleted the iceberg-ref-table branch September 2, 2024 05:20

jaystarshot mentioned this pull request Nov 1, 2024

Add release notes for 0.290 #23936

Merged

25 tasks

tdcmeehan added the from:IBM PR from IBM label Dec 13, 2024

prestodb-ci requested review from a team, auden-woolfson and imsayari404 and removed request for a team December 13, 2024 15:31


		.. code-block:: text

		name \| type \| snapshot_id \| max_reference_age_in_ms \| min_snapshots_to_keep \| max_snapshot_age_in_ms

Comments

Conversation

agrawalreetika commented Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tdcmeehan left a comment

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!

hantangwangd left a comment

Choose a reason for hiding this comment

Uh oh!

agrawalreetika commented Aug 26, 2024

Uh oh!

steveburnett left a comment

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!

hantangwangd left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

agrawalreetika commented Aug 23, 2024 •

edited

Loading