You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "JanKaul (via GitHub)" <gi...@apache.org> on 2023/10/04 10:04:46 UTC
[I] Adopt `Catalog` API to include references to the `TableMetadata` and the `metadata_location` in the `TableCommit` payload for the `update_table` method [iceberg-rust]
JanKaul opened a new issue, #75:
URL: https://github.com/apache/iceberg-rust/issues/75
Iceberg catalogs that make use of a `*.metadata.json` file to store the table metadata require the `metadata_location` and the `TableMetadata` of a Table to perform an `update_table` operation ([see here](https://github.com/JanKaul/iceberg-rust/blob/memory-catalog/crates/iceberg/src/catalog/memory.rs#L176)). It would therefore be helpful to include references to the `metadata_location` and the `TableMetadata` in the `TableCommit` payload of the `update_table` operation.
Something like:
```rust
pub struct TableCommit<'t> {
/// The table ident.
pub ident: TableIdent,
/// Metadata file location of the table
pub metadata_location: &'t str,
/// Table metadata
pub table_metadata: &'t TableMetadata,
/// The requirements of the table.
///
/// Commit will fail if the requirements are not met.
pub requirements: Vec<TableRequirement>,
/// The updates of the table.
pub updates: Vec<TableUpdate>,
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Adopt `Catalog` API to include references to the `TableMetadata` and the `metadata_location` in the `TableCommit` payload for the `update_table` method [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on issue #75:
URL: https://github.com/apache/iceberg-rust/issues/75#issuecomment-1748854108
> It would therefore be helpful to include references to the `metadata_location` and the `TableMetadata` in the [`TableCommit`](https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/catalog/mod.rs#L179) payload of the `update_table` operation.
Could you please provide further explanation on why we require those in `TableCommit`? I was under the impression that the catalog should be responsible for maintaining them. For instance, the catalog can locate the metadata_location and metadata of a table using its TableIdent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Adopt `Catalog` API to include references to the `TableMetadata` and the `metadata_location` in the `TableCommit` payload for the `update_table` method [iceberg-rust]
Posted by "JanKaul (via GitHub)" <gi...@apache.org>.
JanKaul closed issue #75: Adopt `Catalog` API to include references to the `TableMetadata` and the `metadata_location` in the `TableCommit` payload for the `update_table` method
URL: https://github.com/apache/iceberg-rust/issues/75
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Adopt `Catalog` API to include references to the `TableMetadata` and the `metadata_location` in the `TableCommit` payload for the `update_table` method [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on issue #75:
URL: https://github.com/apache/iceberg-rust/issues/75#issuecomment-1750114581
> But in this case the data is already in memory through the `load_table` operation.
You're correct. By reusing the `metadata_location` and `TableMetadata`, we can eliminate 2 unnecessary network requests in this scenario.
However, I have the following concerns about this change:
First of all, not all catalogs are implemented in memory, which means they cannot benefit from this change. Adding `metadata_location` and `TableMetadata` to `CommitTable` could result in additional costs for every update operation.
Secondly, the `Catalog` should not rely on client input. Users may inadvertently fill the `CommitTable` with incorrect metadata location or metadata.
Finally, the location of users' metadata or metadata itself could be outdated. For example, the iceberg table is undergoing concurrent writes and the client's metadata location may have already been changed.
I believe we should exclude `metadata_location` and `TableMetadata` from `CommitTable`. Instead, let's rely on the `Catalog` to maintain the state accurately, which will also enhance the usability of our catalog API.
What are your thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Adopt `Catalog` API to include references to the `TableMetadata` and the `metadata_location` in the `TableCommit` payload for the `update_table` method [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on issue #75:
URL: https://github.com/apache/iceberg-rust/issues/75#issuecomment-1751595418
> > But in this case the data is already in memory through the `load_table` operation.
>
> You're correct. By reusing the `metadata_location` and `TableMetadata`, we can eliminate 2 unnecessary network requests in this scenario.
>
> However, I have the following concerns about this change:
>
> First of all, not all catalogs are implemented in memory, which means they cannot benefit from this change. Adding `metadata_location` and `TableMetadata` to `CommitTable` could result in additional costs for every update operation.
>
> Secondly, the `Catalog` should not rely on client input. Users may inadvertently fill the `CommitTable` with incorrect metadata location or metadata.
>
> Finally, the location of users' metadata or metadata itself could be outdated. For example, the iceberg table is undergoing concurrent writes and the client's metadata location may have already been changed.
>
> I believe we should exclude `metadata_location` and `TableMetadata` from `CommitTable`. Instead, let's rely on the `Catalog` to maintain the state accurately, which will also enhance the usability of our catalog API.
>
> What are your thoughts?
+1, I think catalog should depends on client input for correctness since we may do concurrent modification to table.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Adopt `Catalog` API to include references to the `TableMetadata` and the `metadata_location` in the `TableCommit` payload for the `update_table` method [iceberg-rust]
Posted by "JanKaul (via GitHub)" <gi...@apache.org>.
JanKaul commented on issue #75:
URL: https://github.com/apache/iceberg-rust/issues/75#issuecomment-1750077811
A typical usage would look something like the following:
1. Get `Table` with `load_table` operation. (This already fetches the `metadata_location` and `TableMetadata`)
2. Create transaction with the desired operations
3. Commit transaction with the `update_table` operation
Generally you are right that the catalog could fetch the `metadata_location` and the `TableMetadata`. But in this case the data is already in memory through the `load_table` operation. It would therefore require 2 redundant network requests (one for `metadata_location` and one for `TableMetadata`) that could be avoided. Adding the `metadata_location` and `TableMetadata` references to the `TableCommit` struct just gives the `update_table` operation access to the information that is already there.
Since `metadata_location` and `TableMetadata` are both present in the `Table` struct, it should simply be possible to pass their references to the `CommitTable` struct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org