Add support to redirect table reads from Hive to Iceberg by phd3 · Pull Request #8340 · trinodb/trino

phd3 · 2021-06-21T23:14:27Z

Hive plugin changes on top of #7606

Redirect iceberg table reads from hive catalog to an Iceberg catalog when configured.
Add engine side restrictions for blocking modification operations for redirected tables.

Fixes #4442

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConfig.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

findepi · 2021-06-22T09:37:17Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

Do you mean that redirects on modification operations are a bad idea?
then why do we support them at all?

if they are a good idea in general, why would we want to opt out here?

@findepi

Do you mean that redirects on modification operations are a bad idea?

#7606 (comment)

then why do we support them at all?

does it look better with Fail modifications for redirected tables in engine commit? If so, I can put it as the first commit. Note that Hive connector check is still required because of procedures.

does it look better with Fail modifications for redirected tables in engine commit?

I am not very fluent with this code, but if this implements @electrum 's thinking #7606 (comment), it should go in separate PR and to be debated separately. There hopefully is nothing special about redirects in Iceberg or Hive connector to warrant DDL-specific checks in the connector code (except, maybe, procedures -- which ones?)

makes sense to do it in a separate PR, was trying to gather feedback if exceptions thrown in hive redirection tests feel more natural with this change. and yes, that commit doesn't have anything specific to hive connector.

hive connector checks become sort of "illegal state checks" in non-procedure calls, but wouldn't hurt to still keep proper error message there.

but wouldn't hurt to still keep proper error message there.

except that it suggests -- to the future reader -- that we're coding some connector-specific behavior.

(and I do care about future readers quite a lot:)

Good point. How about changing all connector checks to checkState, and adding special handling for procedures to throw TrinoException?

or TrinoException here, but with a comment. up to you

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveSessionProperties.java

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java

...no-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java

Queries on information_schema.tables failed when the filters pointed to a specific table, and the table was redirected.

Hive Connector redirects Iceberg table reads to the configured Iceberg catalog. Co-authored by: Xingyuan Lin <linxingyuan1102@gmail.com>

phd3 · 2021-07-12T13:51:06Z

Unrelated #8477 and #8345

phd3 · 2021-07-12T13:53:34Z

@findepi @losipiuk @raunaqmorarka @electrum this is ready for review.

i don't remember, long time ago

findepi · 2021-11-08T09:33:41Z

@phd3 can you please rebase?

findepi · 2021-11-09T09:27:04Z

core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java

        Session session = stateMachine.getSession();
        QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getName());
-        Optional<TableHandle> tableHandle = metadata.getTableHandle(session, tableName);
+        Optional<TableHandle> tableHandle = metadata.getOriginalTableHandle(session, tableName, Optional.of(getName()));


Fail modifications for redirected tables in engine

I am not convinced we should to that

here "modifications" is bad wording, meant DDLs. we already added support for DMLs in #8683

The last discussion that we had on this was #7606 (comment)

DDL operations are problematic since users need to be aware of which connector they are using, since they have different data types, partitioning, bucketing, etc. Our thought was that these are relatively rare operations by more advanced users and that trying to have hidden redirections would end up causing more confusion.

do you also feel otherwise for DDLs?

Users sooner or later will ask why ALTER ... ADD COLUMN (something varchar) does not work, and I won't be able to explain to them why not. Yes, from engineer perspective, we may have harder time supporting things like column properties (maybe, or maybe not), but i would assume there are not always used, so users who do not intend to use column properties, will not accept this as a rational explanation for the limitation.
TL;DR yes, DDLs like ALTER .. ADD/DROP COLUMN should be routed as well.

cc @losipiuk @alexjo2144 @claudiusli

findepi · 2021-11-24T16:35:58Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

    }

+    @Override
+    public Optional<CatalogSchemaTableName> redirectTable(ConnectorSession session, SchemaTableName tableName)


Can you please update this to #9870?

Can you please update this to #9870?

I've taken liberty to update the code myself.
I also copied & adapted an awesome test template done by @ssheikin too.
posted #10173 with the changes

These credits belong to @MiguelWeezardo

These credits belong to @MiguelWeezardo

thanks @ssheikin for the comment. added @MiguelWeezardo as co-author in the other PR

raunaqmorarka · 2022-08-02T10:42:39Z

Seems to be superseded by #10173

phd3 marked this pull request as draft June 21, 2021 23:15

phd3 force-pushed the iceberg-redirect-plugin-change branch from 4d6f0f9 to a303baa Compare June 21, 2021 23:48

cla-bot bot added the cla-signed label Jun 21, 2021

findepi requested review from raunaqmorarka and sopel39 June 22, 2021 09:30

findepi previously requested changes Jun 22, 2021

View reviewed changes

raunaqmorarka reviewed Jun 23, 2021

View reviewed changes

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java Outdated Show resolved Hide resolved

raunaqmorarka reviewed Jun 23, 2021

View reviewed changes

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java Outdated Show resolved Hide resolved

raunaqmorarka reviewed Jun 23, 2021

View reviewed changes

...no-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java Outdated Show resolved Hide resolved

findepi requested a review from losipiuk June 23, 2021 12:38

phd3 force-pushed the iceberg-redirect-plugin-change branch from a303baa to 4cd0b46 Compare July 8, 2021 21:52

phd3 added 2 commits July 11, 2021 11:53

Fix failure with table listing for redirected tables

7139d2e

Queries on information_schema.tables failed when the filters pointed to a specific table, and the table was redirected.

Add support to redirect table reads from Hive to Iceberg

d7527b6

Hive Connector redirects Iceberg table reads to the configured Iceberg catalog. Co-authored by: Xingyuan Lin <linxingyuan1102@gmail.com>

phd3 force-pushed the iceberg-redirect-plugin-change branch from 4cd0b46 to bb368d1 Compare July 11, 2021 15:55

Fail modifications for redirected tables in engine

0d7d0b6

phd3 force-pushed the iceberg-redirect-plugin-change branch from bb368d1 to 0d7d0b6 Compare July 11, 2021 16:28

phd3 mentioned this pull request Jul 12, 2021

Failure in TestOraclePoolRemarksReportingConnectorSmokeTest.init #8345

Open

phd3 marked this pull request as ready for review July 12, 2021 13:51

phd3 requested review from electrum, findepi and raunaqmorarka July 12, 2021 13:53

findepi mentioned this pull request Jul 14, 2021

Add support for Trino views in Iceberg connector #8540

Closed

phd3 mentioned this pull request Jul 29, 2021

Handle redirection for Insert, Delete and Update #8683

Merged

findepi force-pushed the master branch from 8538e49 to 1f896ea Compare July 30, 2021 22:13

findepi mentioned this pull request Nov 8, 2021

Add HiveTableRedirectionsProvider extension point #9870

Merged

findepi reviewed Nov 9, 2021

View reviewed changes

findepi reviewed Nov 24, 2021

View reviewed changes

findepi mentioned this pull request Dec 3, 2021

Add support to redirect table reads from Hive to Iceberg #10173

Merged

findinpath mentioned this pull request Jan 12, 2022

Add redirection awareness to the table tasks #10577

Closed

raunaqmorarka closed this Aug 2, 2022

Conversation

phd3 commented Jun 21, 2021 • edited by findepi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phd3 Jul 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

phd3 Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phd3 commented Jul 12, 2021

Uh oh!

phd3 commented Jul 12, 2021

Uh oh!

findepi commented Nov 8, 2021

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!

findepi Nov 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ssheikin Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raunaqmorarka commented Aug 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

phd3 commented Jun 21, 2021 •

edited by findepi

Loading

phd3 Jul 11, 2021 •

edited

Loading

phd3 Jul 12, 2021 •

edited

Loading

findepi Nov 13, 2021 •

edited

Loading

ssheikin Dec 3, 2021 •

edited

Loading