feat: Implement load table api. #89
Conversation
| "Creating table not supported yet!", | ||
| )) | ||
| async fn load_table(&self, table: &TableIdent) -> Result<Table> { | ||
| let request = self |
There was a problem hiding this comment.
What's the typical pattern here in Rust? I would split this out into a function
There was a problem hiding this comment.
Do you mean the construction of http request? I think they are simple enough, and moving them to another method doesn't help much in avoid duplication, how do you think?
There was a problem hiding this comment.
For Python we split it out, because we're also doing some additional things: https://github.com/apache/iceberg-python/blob/main/pyiceberg/catalog/rest.py#L209-L240 But we can refactor this later as well 👍
|
|
||
| let resp = self | ||
| .client | ||
| .query::<LoadTableResponse, ErrorResponse, OK>(request) |
There was a problem hiding this comment.
Do we handle the different error codes?
https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L626-L649
There was a problem hiding this comment.
I took a look at the error response and they all same schema: ErrorModel.
They are all converted to iceberg, and left to user. All necessary information are kept, such as type, code, message, event stacktrace. You can refer to :
| } | ||
|
|
||
| impl Table { | ||
| /// Returns table identifier. |
There was a problem hiding this comment.
Just for context, in PyIceberg we also keep a reference to the config, in case you want to refresh the table.
There was a problem hiding this comment.
I think it's already kept in TableMetadata?
There was a problem hiding this comment.
Oh, you mean reference to Catalog? If so, this may not be desirable since it add life time of Catalog instance to Table.
There was a problem hiding this comment.
In Python this makes a few things easier, but we can also defer that. Easier to add later on, than removing it :D
This pr implements load table in rest catalog, also add accesses to table metadata.
related: #60