Skip to content

fix(catalog/rest): Nessie CreateTable - #308

Merged
zeroshade merged 7 commits into
apache:mainfrom
shubham-tomar:main
Feb 21, 2025
Merged

zeroshade merged 7 commits into
apache:mainfrom
shubham-tomar:main

Conversation

@shubham-tomar

Copy link
Copy Markdown
Contributor

I am using Nessie as a catalog, while creating table i was getting below error

Failed to create table: failed to create table: WebApplicationException: HTTP 400 Bad Request

When i checked nessie logs, i found this error

nessie  | Caused by: com.fasterxml.jackson.databind.exc.ValueInstantiationException: Cannot construct instance of `org.projectnessie.catalog.formats.iceberg.rest.ImmutableIcebergCreateTableRequest`, problem: Cannot build IcebergCreateTableRequest, some of required attributes are not set [stageCreate]

The root cause is:
In the Go library's struct definition, StageCreate is marked as optional with omitempty
When false is set as the value, the JSON marshaler sees it as the zero value for bool and due to omitempty, it omits the field entirely from the JSON
However, the Nessie REST API requires this field to be explicitly set, even if it's false

This small change fixes the issue

@snazy

snazy commented Feb 19, 2025

Copy link
Copy Markdown
Member

The Iceberg REST spec is ambiguous there.
The behaviors for stage-create == true and == false are defined - but no default value/behavior is defined - and the property is optional.
To me that's an issue in the Iceberg REST spec surfaced the Go implementation

@zeroshade zeroshade changed the title Fix: Nessie CreateTable fix(catalog/rest): Nessie CreateTable Feb 19, 2025
@zeroshade

Copy link
Copy Markdown
Member

@snazy Do you know if any other REST implementations have an issue with providing stage-create: false instead of leaving it out?

@snazy

snazy commented Feb 19, 2025

Copy link
Copy Markdown
Member

Do you know if any other REST implementations have an issue with providing stage-create: false instead of leaving it out?

IDK - but it would surprise me if not, because, as I mentioned, a default value isn't defined, so impls would have to guess (wrong)

@shubham-tomar

Copy link
Copy Markdown
Contributor Author

Hi @zeroshade, i have added RenameTable Fix too as discussed

@zeroshade

Copy link
Copy Markdown
Member

@shubham-tomar you forgot to update the unit tests with the changed field names for RenameTable 😄

@shubham-tomar

Copy link
Copy Markdown
Contributor Author

My Bad, updated the PR

@zeroshade
zeroshade merged commit 92d163b into apache:main Feb 21, 2025
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