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

Add support for case-insensitive object names in Iceberg REST catalog #23867

Merged
merged 1 commit into from
Nov 18, 2024
Merged

Add support for case-insensitive object names in Iceberg REST catalog #23867

merged 1 commit into from
Nov 18, 2024

Conversation

mayankvadariya
Copy link
Contributor

@mayankvadariya mayankvadariya commented Oct 22, 2024 •
edited by ebyhr
Loading

Certain Rest catalog implementation(eg. Polaris) supports case-sensitive object(namespace, table, view etc.) names. This change allows querying mixed/upper case letter objects in rest catalog from Trino.
iceberg.rest-catalog.case-insensitive-name-matching controls whether to match lowercase object names in Trino with different case object names in rest catalog with the limitations that only single object name with the same name is supported.

Fixes #23715

Release notes

## Iceberg
* Add support for case-insensitive name matching in REST catalog. ({issue}`23715`)

@cla-bot cla-bot bot added the cla-signed label Oct 22, 2024
@github-actions github-actions bot added docs iceberg Iceberg connector labels Oct 22, 2024
ebyhr
ebyhr reviewed Oct 28, 2024
View reviewed changes
@mayankvadariya mayankvadariya force-pushed the mayank/add-nested-namespace-support-in-rest-catalog branch 3 times, most recently from a5c27dc to 83ca21d Compare October 29, 2024 03:44
@mayankvadariya mayankvadariya requested a review from ebyhr October 29, 2024 20:04
@mayankvadariya mayankvadariya force-pushed the mayank/add-nested-namespace-support-in-rest-catalog branch 2 times, most recently from 181d065 to 6a5a081 Compare November 4, 2024 04:04
@mayankvadariya mayankvadariya requested a review from ebyhr November 4, 2024 14:16
@mayankvadariya
Copy link
Contributor Author

ran newly added tests by modifying ci.yml. All of the trino-iceberg jobs have passed.

one of the trino-iceberg job was cancelled for some unrelated reason https://github.com/trinodb/trino/actions/runs/11674050614/job/32506029684?pr=23867

@mayankvadariya mayankvadariya force-pushed the mayank/add-nested-namespace-support-in-rest-catalog branch from 6e7a8ee to 4af6c9d Compare November 4, 2024 23:35
@ebyhr
Copy link
Member

ebyhr commented Nov 7, 2024

Could you rebase on master to resolve conflicts and squash commits into one?

@mayankvadariya mayankvadariya force-pushed the mayank/add-nested-namespace-support-in-rest-catalog branch 2 times, most recently from 3d5c74e to d247654 Compare November 7, 2024 02:20
@mayankvadariya
Copy link
Contributor Author

@ebyhr CI is passing with squashed commits.

@mayankvadariya
Copy link
Contributor Author

@ebyhr gentle reminder to review this PR.

@ebyhr ebyhr force-pushed the mayank/add-nested-namespace-support-in-rest-catalog branch from d247654 to 68fd499 Compare November 12, 2024 09:47
@ebyhr
Copy link
Member

ebyhr commented Nov 12, 2024 •
edited
Loading

Sorry for my delayed review. Just rebased on master without any changes. I guess there is a logical conflict with #23986.

@mayankvadariya
Copy link
Contributor Author

I think nested namespace support is broken most likely due to apache/iceberg#10858 (comment)

I guess TestIcebergRestCatalogNestedNamespaceConnectorSmokeTest didn't fail in Iceberg bump PR is due to the fact that we use DelegatingRestSessionCatalog there. I think we should change that test to use Polaris.

Meanwhile, do you see any issues if I change TestIcebergPolarisCatalogCaseInsensitiveMapping to use single level namespace instead nested namespace?

@ebyhr
Copy link
Member

ebyhr commented Nov 13, 2024

@mayankvadariya You can use single-level instead.

@mayankvadariya mayankvadariya force-pushed the mayank/add-nested-namespace-support-in-rest-catalog branch from ca3835a to ed61139 Compare November 13, 2024 02:14
@mayankvadariya
Copy link
Contributor Author

@ebyhr CI is green. Please review.

ebyhr
ebyhr reviewed Nov 13, 2024
View reviewed changes
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java Outdated
return remoteView;
}
else if (remoteView.name().equals(schemaTableName.getTableName()) && remoteTable.name().equals(schemaTableName.getTableName())) {
return remoteTable; // table name and view name are same
Copy link
Member

Choose a reason for hiding this comment

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

table name and view name are same

Which metastore supports registering an object as both table and view?

Copy link
Contributor Author

@mayankvadariya mayankvadariya Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Comment seems bit confusing. I'll remove it.
Idea is

  • first if will return if remote table name is different. It may be the case if case-sensitive matching is enabled.
  • second if will return if remote view name is different. It may be the case if case-sensitive matching is enabled.
  • last if will return if remote view and remote table are same. Here, remote table and view name same doesn't imply that remote has table and/or view exist.
    Rather it implies that toRemoteView and toRemoteTable returned same TableIdentifier as that of passed schemaTableName. It happens(through toRemoteObject) when either remote has same table/view name when case-sensitive is enabled or when case-sensitive is disabled.

fyi - In last if, I've ignored comparing remoteTable.namespace() and remoteTable.namespace() as those are expected to be same as toRemoteTable and toRemoteView relies on findRemoteNamespace

@mayankvadariya mayankvadariya force-pushed the mayank/add-nested-namespace-support-in-rest-catalog branch from ed61139 to b101640 Compare November 13, 2024 19:11
@mayankvadariya mayankvadariya requested a review from ebyhr November 13, 2024 20:50
@mayankvadariya
Copy link
Contributor Author

@ebyhr please advise if you have access to rerun the timed out jobs. If not, I can force push to start a new pipeline.

@mayankvadariya
Copy link
Contributor Author

hitting known issue - https://github.com/trinodb/trino/actions/runs/11829268336/job/32960825907?pr=23867#step:5:2449

@ebyhr ebyhr force-pushed the mayank/add-nested-namespace-support-in-rest-catalog branch from af8074d to bbb9834 Compare November 15, 2024 08:04
@mayankvadariya
Copy link
Contributor Author

CI has successfully passed.

@mayankvadariya @ebyhr
Add support for case-insensitive object names in Iceberg REST catalog
6d8163a 
Certain Rest catalog implementation such as Polaris supports
case-sensitive object(namespace, table, view etc.) names. This
change allows querying mixed/upper case letter object names in rest
catalog from Trino.
`iceberg.rest-catalog.case-insensitive-name-matching` controls
whether to match lowercase object names in Trino with different case
object names in rest catalog with the limitations that only single
object name with the identical name is supported.
@ebyhr ebyhr force-pushed the mayank/add-nested-namespace-support-in-rest-catalog branch from b047c90 to 6d8163a Compare November 18, 2024 13:37
@ebyhr ebyhr merged commit 5bd38c1 into trinodb:master Nov 18, 2024
5 of 15 checks passed
@github-actions github-actions bot added this to the 465 milestone Nov 18, 2024
@mosabua mosabua mentioned this pull request Nov 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mosabua mosabua mosabua left review comments

@anusudarsan anusudarsan Awaiting requested review from anusudarsan

@ebyhr ebyhr Awaiting requested review from ebyhr

Assignees
No one assigned
Labels
cla-signed docs iceberg Iceberg connector
Milestone
465
Development

Successfully merging this pull request may close these issues.

Snowflake created objects in external Polaris catalog are not accessible from Trino
3 participants
@mayankvadariya @ebyhr @mosabua