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.

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

Support nested namespaces in Iceberg's REST Catalog #24083

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

denodo-research-labs
Copy link
Contributor

@denodo-research-labs denodo-research-labs commented Nov 19, 2024

Description

Support nested namespaces in Iceberg's REST Catalog. The configuration property iceberg.rest.nested.namespace.enabled enables this feature (default: true).

Motivation and Context

  • Allow Presto to read Snowflake-managed Iceberg tables through external Polaris catalogs when Snowflake identifiers are in lowercase. Snowflake-managed tables are always available in Polaris under nested namespaces.
  • Allow Presto to read Iceberg tables from Unity catalog. Previously an error was thrown:
    Invalid two-part namespace unity.

Impact

Namespaces in Polaris can be nested up to 16 levels. If a large number of recursive namespaces result in lower performance, querying nested namespaces can be disabled: iceberg.rest.nested.namespace.enabled=false

Test Plan

  • Added TestIcebergSmokeRestNestedNamespace that uses the nested namespace: "ns1.ns2" for all operations.
  • We would like to add tests against Polaris catalog, but we couldn't find any hosted container image. This feature request in the Polaris project is still open: [FEATURE REQUEST] Hosted docker image apache/polaris#152
  • Same scenario for the Unity catalog

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

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Iceberg Connector Changes
* Add configuration property ``iceberg.rest.nested.namespace.enabled`` to support nested namespaces in Iceberg's REST Catalog. Defaults to ``true``. :pr:`24083`

steveburnett
steveburnett previously approved these changes Nov 19, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull branch, local doc build, looks good. Thanks for the doc!

@steveburnett
Copy link
Contributor

Suggest revising the release note entry to follow the Order of changes in the Release Notes Guidelines, and formatting.

== RELEASE NOTES ==

Iceberg Connector Changes
* Add configuration property ``iceberg.rest.nested.namespace.enabled`` to support nested namespaces in Iceberg's REST Catalog. Defaults to ``true``. :pr:`24083`

@tdcmeehan tdcmeehan self-assigned this Nov 19, 2024
Copy link
Contributor

@kiersten-stokes kiersten-stokes left a comment

Choose a reason for hiding this comment

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

Thanks for this - I know it's a desired feature! I have one NIT below. My only other doubt is the subclassing of the ~100 tests in TestIcebergSmokeRest as opposed to having a select few tests. I tend to stand on the "more tests the better" side of things but just thinking out loud 🙂

The configuration property iceberg.rest.nested.namespace.enabled allows
nested namespace in Iceberg's REST Catalog
@denodo-research-labs
Copy link
Contributor Author

@kiersten-stokes, we wanted to make sure all features work with nested namespaces, but let us know if you have a better suggestion!

@denodo-research-labs
Copy link
Contributor Author

The failing test, TestIcebergParquetMetadataCaching.testParquetMetadataCaching, has been reported before in #22422

Copy link
Contributor

@kiersten-stokes kiersten-stokes left a comment

Choose a reason for hiding this comment

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

LGTM! Agreed about the tests :)

@tdcmeehan
Copy link
Contributor

@denodo-research-labs can you create issues for creating native tests against Polaris and Unity catalogs, and note the dependent issues that block this? I agree we need such tests and it would be great to not lose track of that.

@tdcmeehan tdcmeehan merged commit ae16c69 into prestodb:master Dec 3, 2024
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants