Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[#6382] feat(iceberg): Make Gravitino Iceberg REST server keep compatibility with Trino #6396

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[#6382] feat(iceberg): Make Gravitino Iceberg REST server keep compatibility with Trino #6396

wants to merge 3 commits into from

Conversation

FANNG1
Copy link
Contributor

@FANNG1 FANNG1 commented Feb 5, 2025 •
edited
Loading

What changes were proposed in this pull request?

To keep compatible with Trino, for the catalog backend which doesn't support view operation:

  1. Return empty list when listing views
  2. Throw NoSuchViewException not UnsupportedOperationException for load view.

Why are the changes needed?

Fix: #6382

Does this PR introduce any user-facing change?

no

How was this patch tested?

test basic SQL with Trino and Gravitino Iceberg REST server

connector.name=iceberg
iceberg.catalog.type=rest
iceberg.rest-catalog.uri=http://192.168.31.176:9001/iceberg/
fs.hadoop.enabled=true

@FANNG1 FANNG1 marked this pull request as draft February 5, 2025 07:52
@FANNG1 FANNG1 changed the title fix view [#6382] feat(iceberg): return empty list when listing view for the catalogs which doesn't support view operation Feb 5, 2025
@FANNG1 FANNG1 changed the title [#6382] feat(iceberg): return empty list when listing view for the catalogs which doesn't support view operation [SIP][#6382] feat(iceberg): return empty list when listing view for the catalogs which doesn't support view operation Feb 5, 2025
@FANNG1 FANNG1 changed the title [SIP][#6382] feat(iceberg): return empty list when listing view for the catalogs which doesn't support view operation [SIP][#6382] feat(iceberg): Make Gravitino Iceberg REST server keep compatibility with Trino Feb 6, 2025
@FANNG1 FANNG1 changed the title [SIP][#6382] feat(iceberg): Make Gravitino Iceberg REST server keep compatibility with Trino [#6382] feat(iceberg): Make Gravitino Iceberg REST server keep compatibility with Trino Feb 6, 2025
@FANNG1 FANNG1 marked this pull request as ready for review February 6, 2025 06:54
@FANNG1 FANNG1 requested a review from theoryxu February 6, 2025 06:55
@FANNG1
Copy link
Contributor Author

FANNG1 commented Feb 6, 2025

@jerryshao @theoryxu @diqiu50 PTAL.

@FANNG1 FANNG1 added the 0.8.0 Release v0.8.0 label Feb 6, 2025
@FANNG1 FANNG1 self-assigned this Feb 6, 2025
diqiu50
diqiu50 approved these changes Feb 6, 2025
View reviewed changes
Copy link
Contributor

@diqiu50 diqiu50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jerryshao
jerryshao reviewed Feb 6, 2025
View reviewed changes
...berg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java
// Trino Analyzer try to get view from Iceberg catalog, return NoSuchViewException to keep
// the compatibility with Trino.
if (!(catalog instanceof ViewCatalog)) {
throw new NoSuchViewException("Catalog %s doesn't support view", catalog.name());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to throw an unsupported exception here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the side of Gravitino, it's reasonable to throw unsupported exception but this will cause Trino query failed, because it will try to get view from Iceberg REST server in analyzer, please refer to https://github.com/trinodb/trino/blob/master/core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java#L2279

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we fail the query if it is not supported? For example, if we write a query with some unsupported features, shall we fail the query, or doing some implicit conversion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause the simple SQL to fail in Trino like select * from xx. The latest Trino code adds a configuration to enable compatibility with REST server that does not support view operations. But for the old trino releases, it couldn't work with Gravitino. This PR moves the compatibility work from client to server, I agree that this is a workaround solution, is more general solution is #6395 but it benefits the cooperation with Trino and Gravitino IRS.

jerryshao
jerryshao reviewed Feb 6, 2025
View reviewed changes
...berg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java
@@ -228,10 +234,18 @@ public void renameView(RenameTableRequest request) {
}

public boolean viewExists(TableIdentifier viewIdentifier) {
if (!(catalog instanceof ViewCatalog)) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I don't know the behavior of Trino, from my feeling, it is better to throw an unsupported exception to let user clearly know the problem, rather than return false.

false can have different meaning, like ViewCatalog is supported but this view is not existed. If we blindly return false, how do we differentiate whether it is not supported or not existed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of the change is to keep consistent with loadView, if the catalog doesn't support the view operation, loadView will throw NoSuchViewException which means view not exists. Yes, this is something odd.

@jerryshao
Copy link
Contributor

jerryshao commented Feb 7, 2025 •
edited
Loading

Hi @jackye1995 , what is your opinion on this? Trino latest master already fixed this, doing a such workaround is to be compatible with the old version of Trino, but I think this is a Trino problem, doing such a workaround on the REST server side seems a little awkward to me. WDYT?

@FANNG1
Copy link
Contributor Author

FANNG1 commented Feb 19, 2025

@jerryshao How about adding a flag to enable the compatibility action with Trino? cc @mchades

@yuqi1129 yuqi1129 added branch-0.8 Automatically cherry-pick commit to branch-0.8 and removed 0.8.0 Release v0.8.0 labels Feb 19, 2025
@FANNG1
Copy link
Contributor Author

FANNG1 commented Feb 21, 2025

trinodb/trino#24815 add iceberg.rest-catalog.view-endpoints-enabled config to keep compatibility with the Iceberg REST server which doesn't support view operations from release 471.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerryshao jerryshao jerryshao left review comments

@diqiu50 diqiu50 diqiu50 approved these changes

@theoryxu theoryxu Awaiting requested review from theoryxu

Assignees

@FANNG1 FANNG1

Labels
branch-0.8 Automatically cherry-pick commit to branch-0.8
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Iceberg Rest Unable to list tables for Hive Metastore with mixed Hive and Iceberg
4 participants
@FANNG1 @jerryshao @diqiu50 @yuqi1129