Add views support in iceberg connector #19872

pratyakshsharma · 2023-06-13T19:33:40Z

Cherry-pick of trinodb/trino@4b4e4f1

Co-authored-by: losipiuk

== RELEASE NOTES ==

Iceberg Changes
* Support view creation via Iceberg connector

yingsu00

@pratyakshsharma Can you please add the cherry-pick information in the commit and PR messages as stated in https://github.com/prestodb/presto/wiki/Review-and-Commit-guidelines#example-commit-message ?

ChunxuTang · 2023-06-18T02:41:16Z

Hi @pratyakshsharma, thanks for your nice contribution!
Just a quick question: For now, the Iceberg connector supports Hive metastore catalog, Hadoop catalog, and Nessie catalog. Will your PR support view creation in all these catalogs or only Hive metastore?

pratyakshsharma · 2023-06-24T09:50:11Z

@ChunxuTang this PR only supports views via hive catalog.

pratyakshsharma · 2023-06-27T09:04:20Z

@yingsu00 This is good to review, all the tests are passing, I will retrigger the cancelled test after your review.

yingsu00

Could you please add the original PR(s) link and original authors in the PR message as well?
Add another commit to back port trinodb/trino@52a2534 before backporting the views changes
The TestIcebergMetadataListing tests change was not backported.
TestIcebergHiveMetadataListing was not backported. Please find the original PR/commit that creates this test and backport it as separate commit(s) before this one.

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

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

yingsu00 · 2023-07-21T05:10:05Z

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

+        MetastoreContext metastoreContext = getMetastoreContext(session);
+        ColumnConverter columnConverter = metastoreContext.getColumnConverter();
+
+        for (ColumnMetadata column : viewMetadata.getColumns()) {


Where does this part of code come from? Strangely, I saw the original commit just added a dummy column. @imjalpreet @agrawalreetika Can you please confirm if this is correct?

@pratyakshsharma Are we adding actual column names here, to keep metadata updated in HMS for views? Or for any other reason?
Do we know in the original commit since its adding dummy column in Table object, how does the metadata looks like there?

So I have followed the way views are created in hive connector in presto. Ultimately, both (view from trino and presto) have the same metadata in HMS as checked in iceberg.information_schema.columns table

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

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

yingsu00 · 2023-07-21T06:20:39Z

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

@@ -206,6 +207,10 @@ public TableMetadata refresh()
            throw new UnknownTableTypeException(getSchemaTableName());
        }

+        if ("true".equalsIgnoreCase(table.getParameters().get(PRESTO_VIEW_FLAG))) {


Are these test failures from previous commits? If so, these fixes need to be in the same commit that introduces the test failure. A general good practice is that there should not be any commit breaks the tests.

Good point, have taken care of it.

presto-product-tests/conf/presto/etc/catalog/iceberg.properties

agrawalreetika · 2023-07-25T05:20:05Z

...ct-tests/src/main/java/com/facebook/presto/tests/iceberg/TestIcebergHiveMetadataListing.java

+                        row(storageTable, "_string"),
+                        row(storageTable, "_integer"));
+    }
+}


@pratyakshsharma Could you please confirm, why are we adding IcebergHiveMetadataListing related tests in this PR?

The original trino commit which is backported for adding views support had changes in this class. Hence I had to backport this class as well so as to have proper test coverage. Also Ying had suggested doing that here #19872 (review).

agrawalreetika · 2023-07-25T05:20:50Z

...ct-tests/src/main/java/com/facebook/presto/tests/iceberg/TestIcebergHiveMetadataListing.java

+                .iterator());
+        onPresto().executeQuery("CREATE TABLE hive.default.hive_table (_double DOUBLE)");
+        onPresto().executeQuery("CREATE VIEW hive.default.hive_view AS SELECT * FROM hive.default.hive_table");
+    }


Why are we creating hive tables and hive views here? Shouldn't it be iceberg tables & views for Iceberg testing?

This is a backport from trinodb/trino@3531676#diff-d45108e44a12da55f3fbda8c8cb9f3ea323ae3caa7f4dddd20fa2535b499a31e

agrawalreetika · 2023-07-25T06:23:25Z

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

+        MetastoreContext metastoreContext = getMetastoreContext(session);
+        ColumnConverter columnConverter = metastoreContext.getColumnConverter();
+
+        for (ColumnMetadata column : viewMetadata.getColumns()) {


@pratyakshsharma Are we adding actual column names here, to keep metadata updated in HMS for views? Or for any other reason?
Do we know in the original commit since its adding dummy column in Table object, how does the metadata looks like there?

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

imjalpreet · 2023-07-28T11:35:32Z

Based on the test suite logs, it is failing when trying to run the profile singlenode-kerberos-hdfs-impersonation, so can we add another iceberg.properties in the respective directory similar to hive.properties.

.../etc/environment-specific-catalogs/singlenode-kerberos-hdfs-impersonation/iceberg.properties

pratyakshsharma · 2023-08-02T17:55:21Z

...ct-tests/src/main/java/com/facebook/presto/tests/iceberg/TestIcebergHiveMetadataListing.java

+                        .add(row("iceberg_table1", "_integer"))
+                        .add(row("iceberg_view", "_string"))
+                        .add(row("iceberg_view", "_integer"))
+                        .add(row("hive_view", "_double"))


@imjalpreet Thank you for the valuable suggestions, product tests are passing finally. I need to verify if we want hive_view to be listed as part of the above query? I feel this is not the desired behaviour.
Trino takes care of this already - https://github.com/trinodb/trino/pull/8153/files#diff-d45108e44a12da55f3fbda8c8cb9f3ea323ae3caa7f4dddd20fa2535b499a31e

pratyakshsharma · 2023-08-02T17:57:13Z

@yingsu00 TestIcebergHiveMetadataListing was introduced as part of this commit in trino - https://github.com/trinodb/trino/pull/8153/commits/35316765099272863f009bbb2ced5d6b4d08aee4#diff-d45108e44a12da55f3fbd[…]3caa7f4dddd20fa2535b499a31e. I have added this as a separate commit but have not included the cherry pick information since I did not backport it completely.

yingsu00 · 2023-08-17T06:10:52Z

@wanglinsong Do you know how to start the 3 yellow tests?

wanglinsong · 2023-08-17T06:26:46Z

@wanglinsong Do you know how to start the 3 yellow tests?

Looks like tests are stuck on circle ci. I don’t have that access.

imjalpreet · 2023-08-17T06:37:05Z

I think we can do a final review and force push to retrigger the tests. These tests were not triggered in circle ci from what I can see.

pratyakshsharma · 2023-09-20T10:43:44Z

@yingsu00 @agrawalreetika @imjalpreet Test failures are unrelated. Please take a pass.

yingsu00 · 2023-09-20T22:01:33Z

Details

Please make sure all tests pass before requesting reviews. Thank you. I reran the tests for you this time, in the future, you can try to re-push to trigger the new run.

pratyakshsharma · 2023-09-27T19:17:30Z

@yingsu00 All checks have passed.

agrawalreetika

Apart from minor nits, overall looks good to me now.

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

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

yingsu00 · 2023-09-29T00:17:24Z

The product tests failed with "failed to register layer: write /usr/libexec/mysqld: no space left on device"
@wanglinsong How do we solve the problem?

yingsu00

Pending production tests

skairali · 2023-10-01T14:22:07Z

@yingsu00 this is a required fix for all enterprise presto consumers

agrawalreetika · 2023-10-02T12:10:24Z

@pratyakshsharma Rebase your PR on master, this #21000 may help for product case space related failures.

steveburnett

I have a couple of small suggestions to consider for the docs. Let me know what you think of them, please.

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

Cherry-pick of trinodb/trino@4b6297e Co-authored-by: electrum

Cherry-pick of trinodb/trino@4b4e4f1 Co-authored-by: losipiuk

steveburnett

Requested docs change present and verified in local docs build.

LGTM! (docs)

pratyakshsharma requested a review from a team as a code owner June 13, 2023 19:33

pratyakshsharma requested a review from presto-oss June 13, 2023 19:33

yingsu00 reviewed Jun 16, 2023

View reviewed changes

pratyakshsharma force-pushed the iceberg-view branch from ebceeb2 to d94093a Compare June 24, 2023 09:49

pratyakshsharma force-pushed the iceberg-view branch 2 times, most recently from b67abc4 to 243c9c1 Compare June 27, 2023 05:41

yingsu00 requested changes Jun 30, 2023

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java Outdated Show resolved Hide resolved

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestCase.java Outdated Show resolved Hide resolved

yingsu00 self-assigned this Jun 30, 2023

pratyakshsharma force-pushed the iceberg-view branch 2 times, most recently from fe95211 to 5c8dc37 Compare July 19, 2023 13:10

yingsu00 reviewed Jul 21, 2023

View reviewed changes

yingsu00 requested review from imjalpreet, agrawalreetika and yingsu00 July 21, 2023 06:21

imjalpreet reviewed Jul 21, 2023

View reviewed changes

presto-product-tests/conf/presto/etc/catalog/iceberg.properties Show resolved Hide resolved

agrawalreetika requested changes Jul 25, 2023

View reviewed changes

imjalpreet reviewed Aug 2, 2023

View reviewed changes

.../etc/environment-specific-catalogs/singlenode-kerberos-hdfs-impersonation/iceberg.properties Show resolved Hide resolved

pratyakshsharma force-pushed the iceberg-view branch from 63ddf55 to 75af12b Compare August 2, 2023 11:58

pratyakshsharma commented Aug 2, 2023

View reviewed changes

pratyakshsharma force-pushed the iceberg-view branch from 35653fb to 6e8ec0c Compare August 3, 2023 12:09

yingsu00 mentioned this pull request Aug 7, 2023

Add Presto view support in Iceberg connector #20439

Open

yingsu00 added the iceberg Apache Iceberg related label Aug 10, 2023

pratyakshsharma force-pushed the iceberg-view branch 4 times, most recently from 0a99fa5 to c0f8f3e Compare September 19, 2023 19:50

pratyakshsharma force-pushed the iceberg-view branch 2 times, most recently from 807fdd3 to b95158d Compare September 20, 2023 17:33

pratyakshsharma mentioned this pull request Sep 23, 2023

Fix handling of TableNotFoundException in getTableHandle() in IcebergHiveMetadata #20952

Open

pratyakshsharma force-pushed the iceberg-view branch 3 times, most recently from 4c7e7aa to a3d39ba Compare September 27, 2023 15:34

agrawalreetika approved these changes Sep 28, 2023

View reviewed changes

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergHiveMetadata.java Outdated Show resolved Hide resolved

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java Outdated Show resolved Hide resolved

pratyakshsharma force-pushed the iceberg-view branch 2 times, most recently from 7d1c9ed to 8262d80 Compare September 28, 2023 10:57

yingsu00 approved these changes Sep 29, 2023

View reviewed changes

steveburnett requested changes Oct 2, 2023

View reviewed changes

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

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

Move buildInitialPrivilegeSet to MetastoreUtil

04b89f8

Cherry-pick of trinodb/trino@4b6297e Co-authored-by: electrum

pratyakshsharma force-pushed the iceberg-view branch 2 times, most recently from 35459ce to 74569fe Compare October 3, 2023 11:50

pratyakshsharma added 2 commits October 3, 2023 18:57

Add TestIcebergHiveMetadataListing.java class

6b6f4ae

Add views support in iceberg connector

b11870b

Cherry-pick of trinodb/trino@4b4e4f1 Co-authored-by: losipiuk

pratyakshsharma force-pushed the iceberg-view branch from 74569fe to b11870b Compare October 3, 2023 13:28

steveburnett approved these changes Oct 3, 2023

View reviewed changes

yingsu00 merged commit 154ceeb into prestodb:master Oct 5, 2023

wanglinsong mentioned this pull request Dec 8, 2023

Add release notes for 0.285 #21500

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add views support in iceberg connector #19872

Add views support in iceberg connector #19872

pratyakshsharma commented Jun 13, 2023 •

edited

Loading

yingsu00 left a comment

ChunxuTang commented Jun 18, 2023

pratyakshsharma commented Jun 24, 2023

pratyakshsharma commented Jun 27, 2023

yingsu00 left a comment

yingsu00 Jul 21, 2023

agrawalreetika Jul 25, 2023

pratyakshsharma Aug 9, 2023

yingsu00 Jul 21, 2023

pratyakshsharma Aug 3, 2023

agrawalreetika Jul 25, 2023

pratyakshsharma Aug 3, 2023

agrawalreetika Jul 25, 2023

pratyakshsharma Aug 3, 2023

agrawalreetika Jul 25, 2023

imjalpreet commented Jul 28, 2023

pratyakshsharma Aug 2, 2023

pratyakshsharma commented Aug 2, 2023

yingsu00 commented Aug 17, 2023

wanglinsong commented Aug 17, 2023

imjalpreet commented Aug 17, 2023

pratyakshsharma commented Sep 20, 2023

yingsu00 commented Sep 20, 2023 •

edited

Loading

pratyakshsharma commented Sep 27, 2023

agrawalreetika left a comment

yingsu00 commented Sep 29, 2023

yingsu00 left a comment

skairali commented Oct 1, 2023

agrawalreetika commented Oct 2, 2023

steveburnett left a comment

steveburnett left a comment

Add views support in iceberg connector #19872

Add views support in iceberg connector #19872

Conversation

pratyakshsharma commented Jun 13, 2023 • edited Loading

yingsu00 left a comment

Choose a reason for hiding this comment

ChunxuTang commented Jun 18, 2023

pratyakshsharma commented Jun 24, 2023

pratyakshsharma commented Jun 27, 2023

yingsu00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imjalpreet commented Jul 28, 2023

Choose a reason for hiding this comment

pratyakshsharma commented Aug 2, 2023

yingsu00 commented Aug 17, 2023

wanglinsong commented Aug 17, 2023

imjalpreet commented Aug 17, 2023

pratyakshsharma commented Sep 20, 2023

yingsu00 commented Sep 20, 2023 • edited Loading

pratyakshsharma commented Sep 27, 2023

agrawalreetika left a comment

Choose a reason for hiding this comment

yingsu00 commented Sep 29, 2023

yingsu00 left a comment

Choose a reason for hiding this comment

skairali commented Oct 1, 2023

agrawalreetika commented Oct 2, 2023

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

pratyakshsharma commented Jun 13, 2023 •

edited

Loading

yingsu00 commented Sep 20, 2023 •

edited

Loading