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

Fix setting iceberg table location on creation for REST/NESSIE catalog #24218

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

hantangwangd
Copy link
Member

@hantangwangd hantangwangd commented Dec 7, 2024

Description

Currently we ignore the specified table property location when creating tables on iceberg catalogs based on IcebergNativeMetadata, this will result in that the newly created table's location always using default values, regardless of whether a custom location is explicitly specified or not.

This behavior is somewhat unreasonable, as for catalogs that support specifying table locations on creation (like Rest and Nessie), the newly created table's location should be the specified path; while for catalogs that do not support specifying table locations on creation (like Hadoop), exceptions should be thrown directly.

This PR fix the above problem, pass the specified table location to Iceberg lib when creating tables on catalogs based on IcebergNativeMetadata .

Motivation and Context

Support specifying table location on creation for Iceberg connector on as many catalog types as possible.

Impact

Iceberg connector configured with REST and NESSIE catalogs now support specifying table location on creation.
Iceberg connector configured with HADOOP catalogs now fail directly when specifying table location on creation.

Test Plan

  • Newly added test cases for creating iceberg table with custom location

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • 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

== RELEASE NOTES ==

Iceberg Connector Changes
* Add support of specifying table location on creation for Iceberg connector when configured with ``REST`` and ``NESSIE``. :pr:`24218`

@hantangwangd hantangwangd force-pushed the fix_set_table_location branch from 210ae18 to 0b03f7a Compare December 7, 2024 18:55
@hantangwangd hantangwangd marked this pull request as ready for review December 8, 2024 01:25
@hantangwangd hantangwangd requested review from ZacBlanco and a team as code owners December 8, 2024 01:25
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.

This has always bothered me - LGTM with one NIT

getLocation(schemaName, "test_create_table_like_original") :
getLocation(schemaName, "test_create_table_like_copy4")));
dropTable(session, "test_create_table_like_copy4");
if (!catalogType.equals(CatalogType.HADOOP)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: should we go with the fully qualified import here? (and I think for some instances of CatalogType.HIVE in the same file)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

@tdcmeehan tdcmeehan self-assigned this Dec 10, 2024
@tdcmeehan tdcmeehan merged commit d4e878b into prestodb:master Dec 11, 2024
57 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.

3 participants