Add support for 'overwrite' option in register_table#3290
Conversation
|
Looks good to me, one small nit would be to add some tests in integration tests here if the iceberg rest image supports the overwriting flag |
|
The current test target (1.10.0) supports the option in OpenAPI yml, but the catalog doesn't support it. We can add an integration test once 1.11.0 (apache/iceberg#15525) is released. |
ebyhr
changed the title
|
any chance for this to get merged, currently, i am doing tuncate and add files with other tool, and it is a pain as it is not atomic |
| Args: | ||
| identifier (Union[str, Identifier]): Table identifier for the table | ||
| metadata_location (str): The location to the metadata | ||
| overwrite (bool): Whether to overwrite the existing table, default False |
There was a problem hiding this comment.
should we add NotImplementedError here too?
There was a problem hiding this comment.
This method in dynamodb and noop always raises NotImplementedError. Do we still need a separate logc for the overwrite argument?
There was a problem hiding this comment.
I see NotImplementedError in other implementations but not for dynamodb/noop
is that intended?
| Args: | ||
| identifier (Union[str, Identifier]): Table identifier for the table | ||
| metadata_location (str): The location to the metadata | ||
| overwrite (bool): Whether to overwrite the existing table, default False |
There was a problem hiding this comment.
nit: raise NotImplementedError here too?
Right now we're locked at an older version of the Image: #3240. Once there is a new Java release, we can definitly add this test.
@djouallah Since everybody approved, I went ahead and merged this 👍 Thanks @ebyhr for working on this, and thanks @ndrluis and @kevinjqliu for the review 🚀 |
6 participants
Rationale for this change
Are these changes tested?
Are there any user-facing changes?
This PR adds
overwriteparameter tocatalog.register_tablemethod.