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

REST: Set fresh IDs on create-table #327

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Jan 30, 2024

For the other implementations, we do this when writing the metadata, but for the REST catalog, we post the schema itself.

The REST spec does not mention anything about setting fresh IDs, so it is best to do it ourselves.

For the other implementation we do this when writing
the metadata, but for the REST catalog we just post
the schema itself.

The REST spec does not mention anything about
setting fresh IDs, so it is best to do it ourselves.

namespace_and_table = self._split_identifier_for_path(identifier)
request = CreateTableRequest(
name=namespace_and_table["table"],
location=location,
table_schema=schema,
table_schema=iceberg_schema,
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this done on the server side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but some server sides still require the IDs to be unique

Copy link
Contributor

@rdblue rdblue left a comment

Choose a reason for hiding this comment

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

I think this shouldn't cause any issues and is probably the safest option, but the REST catalog implementation is very likely already doing this itself (and should).

@Fokko Fokko merged commit f69b231 into apache:main Jan 30, 2024
6 checks passed
@Fokko
Copy link
Contributor Author

Fokko commented Jan 30, 2024

Thanks for the prompt review @rdblue

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.

2 participants