Fokko commented on code in PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89#discussion_r1377882393
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -312,11 +316,43 @@ impl Catalog for RestCatalog {
}
/// Load table from the catalog.
- async fn load_table(&self, _table: &TableIdent) -> Result<Table> {
- Err(Error::new(
- ErrorKind::FeatureUnsupported,
- "Creating table not supported yet!",
- ))
+ async fn load_table(&self, table: &TableIdent) -> Result<Table> {
+ let request = self
+ .client
+ .0
+ .get(self.config.table_endpoint(table))
+ .build()?;
+
+ let resp = self
+ .client
+ .query::<LoadTableResponse, ErrorResponse, OK>(request)
+ .await?;
+
+ let mut props = self.config.props.clone();
+ if let Some(config) = resp.config {
+ props.extend(config);
+ }
+
+ let file_io = match self
+ .config
+ .warehouse
Review Comment:
Nit: In PyIceberg we use the metadata location to determine the location.
The warehouse config key might not be set?
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -312,11 +316,43 @@ impl Catalog for RestCatalog {
}
/// Load table from the catalog.
- async fn load_table(&self, _table: &TableIdent) -> Result<Table> {
- Err(Error::new(
- ErrorKind::FeatureUnsupported,
- "Creating table not supported yet!",
- ))
+ async fn load_table(&self, table: &TableIdent) -> Result<Table> {
+ let request = self
Review Comment:
What's the typical pattern here in Rust? I would split this out into a
function
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -312,11 +316,43 @@ impl Catalog for RestCatalog {
}
/// Load table from the catalog.
- async fn load_table(&self, _table: &TableIdent) -> Result<Table> {
- Err(Error::new(
- ErrorKind::FeatureUnsupported,
- "Creating table not supported yet!",
- ))
+ async fn load_table(&self, table: &TableIdent) -> Result<Table> {
+ let request = self
+ .client
+ .0
+ .get(self.config.table_endpoint(table))
+ .build()?;
+
+ let resp = self
+ .client
+ .query::<LoadTableResponse, ErrorResponse, OK>(request)
+ .await?;
+
+ let mut props = self.config.props.clone();
+ if let Some(config) = resp.config {
+ props.extend(config);
+ }
+
+ let file_io = match self
+ .config
+ .warehouse
+ .as_ref()
+ .or_else(|| resp.metadata_location.as_ref())
+ {
+ Some(url) => FileIO::from_path(url)?.with_props(props).build()?,
+ None => FileIOBuilder::new("s3").with_props(props).build()?,
Review Comment:
I agree, I think it makes more sense to raise an exception.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -312,11 +316,43 @@ impl Catalog for RestCatalog {
}
/// Load table from the catalog.
- async fn load_table(&self, _table: &TableIdent) -> Result<Table> {
- Err(Error::new(
- ErrorKind::FeatureUnsupported,
- "Creating table not supported yet!",
- ))
+ async fn load_table(&self, table: &TableIdent) -> Result<Table> {
+ let request = self
+ .client
+ .0
+ .get(self.config.table_endpoint(table))
+ .build()?;
+
+ let resp = self
+ .client
+ .query::<LoadTableResponse, ErrorResponse, OK>(request)
Review Comment:
Do we handle the different error codes?
https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L626-L649
##########
crates/iceberg/src/table.rs:
##########
@@ -17,10 +17,33 @@
//! Table API for Apache Iceberg
+use crate::io::FileIO;
use crate::spec::TableMetadata;
+use crate::TableIdent;
+use typed_builder::TypedBuilder;
/// Table represents a table in the catalog.
+#[derive(TypedBuilder)]
pub struct Table {
- metadata_location: String,
+ file_io: FileIO,
+ #[builder(default, setter(strip_option))]
+ metadata_location: Option<String>,
metadata: TableMetadata,
+ identifier: TableIdent,
+}
+
+impl Table {
+ /// Returns table identifier.
Review Comment:
Just for context, in PyIceberg we also keep a reference to the config, in
case you want to refresh the table.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -312,11 +316,43 @@ impl Catalog for RestCatalog {
}
/// Load table from the catalog.
- async fn load_table(&self, _table: &TableIdent) -> Result<Table> {
- Err(Error::new(
- ErrorKind::FeatureUnsupported,
- "Creating table not supported yet!",
- ))
+ async fn load_table(&self, table: &TableIdent) -> Result<Table> {
+ let request = self
+ .client
+ .0
+ .get(self.config.table_endpoint(table))
+ .build()?;
+
+ let resp = self
+ .client
+ .query::<LoadTableResponse, ErrorResponse, OK>(request)
+ .await?;
+
+ let mut props = self.config.props.clone();
+ if let Some(config) = resp.config {
+ props.extend(config);
Review Comment:
Next to the `response.config`, we also want to merge in the
`response.metadata.config`. See:
https://github.com/apache/iceberg-python/blob/main/pyiceberg/catalog/rest.py#L424
In Iceberg you can also configure the FileIO by setting properties on the
table itself.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
