You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "liurenjie1024 (via GitHub)" <gi...@apache.org> on 2023/10/31 03:06:35 UTC
[PR] feat: Implement load table api. [iceberg-rust]
liurenjie1024 opened a new pull request, #89:
URL: https://github.com/apache/iceberg-rust/pull/89
This pr implements load table in rest catalog, also add accesses to table metadata.
--
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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89#discussion_r1386224255
##########
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:
In Python this makes a few things easier, but we can also defer that. Easier to add later on, than removing it :D
--
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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89#discussion_r1378532556
##########
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:
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 : https://github.com/apache/iceberg-rust/blob/6b530f5ae76806562a820e9222b4557876be17b0/crates/catalog/rest/src/catalog.rs#L486
--
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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89#discussion_r1377416089
##########
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:
Take `s3` as default seems not cool, how could we support azblob, gcs and so on?
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -109,50 +110,139 @@ pub struct TableMetadata {
}
impl TableMetadata {
+ /// Returns format version of this metadata.
+ #[inline]
+ pub fn format_version(&self) -> FormatVersion {
Review Comment:
Maybe we can make `FormatVersion` copy?
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -109,50 +110,139 @@ pub struct TableMetadata {
}
impl TableMetadata {
+ /// Returns format version of this metadata.
+ #[inline]
+ pub fn format_version(&self) -> FormatVersion {
+ self.format_version.clone()
+ }
+
+ /// Returns uuid of current table.
+ #[inline]
+ pub fn uuid(&self) -> &Uuid {
Review Comment:
`Uuid` is Copy.
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -109,50 +110,139 @@ pub struct TableMetadata {
}
impl TableMetadata {
+ /// Returns format version of this metadata.
+ #[inline]
+ pub fn format_version(&self) -> FormatVersion {
+ self.format_version.clone()
+ }
+
+ /// Returns uuid of current table.
+ #[inline]
+ pub fn uuid(&self) -> &Uuid {
+ &self.table_uuid
+ }
+
+ /// Returns table location.
+ #[inline]
+ pub fn location(&self) -> &str {
+ self.location.as_str()
+ }
+
+ /// Returns last sequence number.
+ #[inline]
+ pub fn last_sequence_number(&self) -> i64 {
+ self.last_sequence_number
+ }
+
+ /// Returns last updated time.
+ #[inline]
+ pub fn last_updated_ms(&self) -> i64 {
Review Comment:
Could we return a Datetime for simpler usage?
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -109,50 +110,139 @@ pub struct TableMetadata {
}
impl TableMetadata {
+ /// Returns format version of this metadata.
+ #[inline]
+ pub fn format_version(&self) -> FormatVersion {
+ self.format_version.clone()
+ }
+
+ /// Returns uuid of current table.
+ #[inline]
+ pub fn uuid(&self) -> &Uuid {
+ &self.table_uuid
+ }
+
+ /// Returns table location.
+ #[inline]
+ pub fn location(&self) -> &str {
+ self.location.as_str()
+ }
+
+ /// Returns last sequence number.
+ #[inline]
+ pub fn last_sequence_number(&self) -> i64 {
+ self.last_sequence_number
+ }
+
+ /// Returns last updated time.
+ #[inline]
+ pub fn last_updated_ms(&self) -> i64 {
+ self.last_updated_ms
+ }
+
+ /// Returns schemas
+ #[inline]
+ pub fn schemas(&self) -> impl Iterator<Item = &SchemaRef> {
+ self.schemas.values()
+ }
+
+ /// Lookup schema by id.
+ #[inline]
+ pub fn schema_by_id(&self, schema_id: i32) -> Option<&SchemaRef> {
+ self.schemas.get(&schema_id)
+ }
+
/// Get current schema
#[inline]
- pub fn current_schema(&self) -> Result<Arc<Schema>, Error> {
- self.schemas
- .get(&self.current_schema_id)
- .ok_or_else(|| {
- Error::new(
- ErrorKind::DataInvalid,
- format!("Schema id {} not found!", self.current_schema_id),
- )
- })
- .cloned()
+ pub fn current_schema(&self) -> &SchemaRef {
+ self.schema_by_id(self.current_schema_id)
+ .expect("Current schema id set, but not found in table metadata")
+ }
+
+ /// Returns all partition specs.
+ #[inline]
+ pub fn partition_specs(&self) -> impl Iterator<Item = &PartitionSpecRef> {
+ self.partition_specs.values()
}
+
+ /// Lookup partition spec by id.
+ #[inline]
+ pub fn partition_spec_by_id(&self, spec_id: i32) -> Option<&PartitionSpecRef> {
+ self.partition_specs.get(&spec_id)
+ }
+
/// Get default partition spec
#[inline]
- pub fn default_partition_spec(&self) -> Result<&PartitionSpec, Error> {
- self.partition_specs
- .get(&self.default_spec_id)
- .ok_or_else(|| {
- Error::new(
- ErrorKind::DataInvalid,
- format!("Partition spec id {} not found!", self.default_spec_id),
- )
- })
+ pub fn default_partition_spec(&self) -> Option<&PartitionSpecRef> {
+ if self.default_spec_id == DEFAULT_SPEC_ID {
+ self.partition_spec_by_id(DEFAULT_SPEC_ID)
+ } else {
+ Some(
+ self.partition_spec_by_id(DEFAULT_SPEC_ID)
+ .expect("Default partition spec id set, but not found in table metadata"),
+ )
+ }
+ }
+
+ /// Returns all snapshots
+ #[inline]
+ pub fn snapshots(&self) -> impl Iterator<Item = &SnapshotRef> {
Review Comment:
Will `impl Iterator<Item = SnapshotRef>` simpler to use? Or just returns `Vec<SnapshotRef>`? It there a case that users want to have `snapshots` but not collect them? Or we could provide `snapshots` and `snapshots_iter`?
The same question applies to all public API about table metadata.
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -109,50 +110,139 @@ pub struct TableMetadata {
}
impl TableMetadata {
+ /// Returns format version of this metadata.
+ #[inline]
+ pub fn format_version(&self) -> FormatVersion {
+ self.format_version.clone()
+ }
+
+ /// Returns uuid of current table.
+ #[inline]
+ pub fn uuid(&self) -> &Uuid {
+ &self.table_uuid
+ }
+
+ /// Returns table location.
+ #[inline]
+ pub fn location(&self) -> &str {
+ self.location.as_str()
+ }
+
+ /// Returns last sequence number.
+ #[inline]
+ pub fn last_sequence_number(&self) -> i64 {
+ self.last_sequence_number
+ }
+
+ /// Returns last updated time.
+ #[inline]
+ pub fn last_updated_ms(&self) -> i64 {
+ self.last_updated_ms
+ }
+
+ /// Returns schemas
+ #[inline]
+ pub fn schemas(&self) -> impl Iterator<Item = &SchemaRef> {
+ self.schemas.values()
+ }
+
+ /// Lookup schema by id.
+ #[inline]
+ pub fn schema_by_id(&self, schema_id: i32) -> Option<&SchemaRef> {
+ self.schemas.get(&schema_id)
+ }
+
/// Get current schema
#[inline]
- pub fn current_schema(&self) -> Result<Arc<Schema>, Error> {
- self.schemas
- .get(&self.current_schema_id)
- .ok_or_else(|| {
- Error::new(
- ErrorKind::DataInvalid,
- format!("Schema id {} not found!", self.current_schema_id),
- )
- })
- .cloned()
+ pub fn current_schema(&self) -> &SchemaRef {
+ self.schema_by_id(self.current_schema_id)
+ .expect("Current schema id set, but not found in table metadata")
+ }
+
+ /// Returns all partition specs.
+ #[inline]
+ pub fn partition_specs(&self) -> impl Iterator<Item = &PartitionSpecRef> {
+ self.partition_specs.values()
}
+
+ /// Lookup partition spec by id.
+ #[inline]
+ pub fn partition_spec_by_id(&self, spec_id: i32) -> Option<&PartitionSpecRef> {
Review Comment:
The same as `SchemaRef`.
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -109,50 +110,139 @@ pub struct TableMetadata {
}
impl TableMetadata {
+ /// Returns format version of this metadata.
+ #[inline]
+ pub fn format_version(&self) -> FormatVersion {
+ self.format_version.clone()
+ }
+
+ /// Returns uuid of current table.
+ #[inline]
+ pub fn uuid(&self) -> &Uuid {
+ &self.table_uuid
+ }
+
+ /// Returns table location.
+ #[inline]
+ pub fn location(&self) -> &str {
+ self.location.as_str()
+ }
+
+ /// Returns last sequence number.
+ #[inline]
+ pub fn last_sequence_number(&self) -> i64 {
+ self.last_sequence_number
+ }
+
+ /// Returns last updated time.
+ #[inline]
+ pub fn last_updated_ms(&self) -> i64 {
+ self.last_updated_ms
+ }
+
+ /// Returns schemas
+ #[inline]
+ pub fn schemas(&self) -> impl Iterator<Item = &SchemaRef> {
+ self.schemas.values()
+ }
+
+ /// Lookup schema by id.
+ #[inline]
+ pub fn schema_by_id(&self, schema_id: i32) -> Option<&SchemaRef> {
+ self.schemas.get(&schema_id)
+ }
+
/// Get current schema
#[inline]
- pub fn current_schema(&self) -> Result<Arc<Schema>, Error> {
- self.schemas
- .get(&self.current_schema_id)
- .ok_or_else(|| {
- Error::new(
- ErrorKind::DataInvalid,
- format!("Schema id {} not found!", self.current_schema_id),
- )
- })
- .cloned()
+ pub fn current_schema(&self) -> &SchemaRef {
+ self.schema_by_id(self.current_schema_id)
+ .expect("Current schema id set, but not found in table metadata")
+ }
+
+ /// Returns all partition specs.
+ #[inline]
+ pub fn partition_specs(&self) -> impl Iterator<Item = &PartitionSpecRef> {
+ self.partition_specs.values()
}
+
+ /// Lookup partition spec by id.
+ #[inline]
+ pub fn partition_spec_by_id(&self, spec_id: i32) -> Option<&PartitionSpecRef> {
+ self.partition_specs.get(&spec_id)
+ }
+
/// Get default partition spec
#[inline]
- pub fn default_partition_spec(&self) -> Result<&PartitionSpec, Error> {
- self.partition_specs
- .get(&self.default_spec_id)
- .ok_or_else(|| {
- Error::new(
- ErrorKind::DataInvalid,
- format!("Partition spec id {} not found!", self.default_spec_id),
- )
- })
+ pub fn default_partition_spec(&self) -> Option<&PartitionSpecRef> {
+ if self.default_spec_id == DEFAULT_SPEC_ID {
+ self.partition_spec_by_id(DEFAULT_SPEC_ID)
+ } else {
+ Some(
+ self.partition_spec_by_id(DEFAULT_SPEC_ID)
+ .expect("Default partition spec id set, but not found in table metadata"),
+ )
+ }
+ }
+
+ /// Returns all snapshots
+ #[inline]
+ pub fn snapshots(&self) -> impl Iterator<Item = &SnapshotRef> {
+ self.snapshots.values()
+ }
+
+ /// Lookup snapshot by id.
+ #[inline]
+ pub fn snapshot_by_id(&self, snapshot_id: i64) -> Option<&SnapshotRef> {
+ self.snapshots.get(&snapshot_id)
+ }
+
+ /// Returns snapshot history.
+ #[inline]
+ pub fn history(&self) -> &[SnapshotLog] {
+ &self.snapshot_log
}
/// Get current snapshot
#[inline]
- pub fn current_snapshot(&self) -> Result<Option<Arc<Snapshot>>, Error> {
- match (&self.current_snapshot_id, &self.snapshots) {
- (Some(snapshot_id), Some(snapshots)) => Ok(snapshots.get(snapshot_id).cloned()),
- (Some(-1), None) => Ok(None),
- (None, None) => Ok(None),
- (Some(_), None) => Err(Error::new(
- ErrorKind::DataInvalid,
- "Snapshot id is provided but there are no snapshots".to_string(),
- )),
- (None, Some(_)) => Err(Error::new(
- ErrorKind::DataInvalid,
- "There are snapshots but no snapshot id is provided".to_string(),
- )),
+ pub fn current_snapshot(&self) -> Option<&SnapshotRef> {
+ self.current_snapshot_id.map(|s| {
+ self.snapshot_by_id(s)
+ .expect("Current snapshot id has been set, but doesn't exist in metadata")
+ })
+ }
+
+ /// Return all sort orders.
+ #[inline]
+ pub fn sort_orders(&self) -> impl Iterator<Item = &SortOrderRef> {
+ self.sort_orders.values()
+ }
+
+ /// Lookup sort order by id.
+ #[inline]
+ pub fn sort_order_by_id(&self, sort_order_id: i64) -> Option<&SortOrderRef> {
+ self.sort_orders.get(&sort_order_id)
+ }
+
+ /// Returns default sort order id.
+ #[inline]
+ pub fn default_sort_order(&self) -> Option<&SortOrderRef> {
+ if self.default_sort_order_id == DEFAULT_SORT_ORDER_ID {
+ self.sort_orders.get(&DEFAULT_SORT_ORDER_ID)
+ } else {
+ Some(
+ self.sort_orders
+ .get(&self.default_sort_order_id)
+ .expect("Default order id has been set, but not found in table metadata!"),
+ )
}
}
+ /// Returns properties of table.
+ #[inline]
+ pub fn properties(&self) -> &HashMap<String, String> {
Review Comment:
I readly want rust has read_only public fields...
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -174,23 +264,9 @@ impl TableMetadata {
)
});
- if let Some(snapshots) = &mut self.snapshots {
- self.snapshot_log.push(snapshot.log());
- snapshots.insert(snapshot.snapshot_id(), Arc::new(snapshot));
- } else {
- if !self.snapshot_log.is_empty() {
- return Err(Error::new(
- ErrorKind::DataInvalid,
- "Snapshot logs is empty while snapshots is not!",
- ));
- }
-
- self.snapshot_log = vec![snapshot.log()];
- self.snapshots = Some(HashMap::from_iter(vec![(
- snapshot.snapshot_id(),
- Arc::new(snapshot),
- )]));
- }
+ self.snapshot_log.push(snapshot.log());
+ self.snapshots
+ .insert(snapshot.snapshot_id(), Arc::new(snapshot));
Review Comment:
Maybe we can change this API into non-failable?
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -109,50 +110,139 @@ pub struct TableMetadata {
}
impl TableMetadata {
+ /// Returns format version of this metadata.
+ #[inline]
+ pub fn format_version(&self) -> FormatVersion {
+ self.format_version.clone()
+ }
+
+ /// Returns uuid of current table.
+ #[inline]
+ pub fn uuid(&self) -> &Uuid {
+ &self.table_uuid
+ }
+
+ /// Returns table location.
+ #[inline]
+ pub fn location(&self) -> &str {
+ self.location.as_str()
+ }
+
+ /// Returns last sequence number.
+ #[inline]
+ pub fn last_sequence_number(&self) -> i64 {
+ self.last_sequence_number
+ }
+
+ /// Returns last updated time.
+ #[inline]
+ pub fn last_updated_ms(&self) -> i64 {
+ self.last_updated_ms
+ }
+
+ /// Returns schemas
+ #[inline]
+ pub fn schemas(&self) -> impl Iterator<Item = &SchemaRef> {
+ self.schemas.values()
+ }
+
+ /// Lookup schema by id.
+ #[inline]
+ pub fn schema_by_id(&self, schema_id: i32) -> Option<&SchemaRef> {
Review Comment:
`SchemaRef` is a `Arc` that cheap to clone, how about returning `SchemaRef` directly instead of `&SchemaRef`?
--
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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89#issuecomment-1791793211
CC @Xuanwo @Fokko Any other comments?
--
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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89#discussion_r1379743215
##########
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:
Oh, you mean reference to `Catalog`? If so, this may not be desirable since it add life time of `Catalog` instance 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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89#discussion_r1378522771
##########
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:
This code here first try with `warehouse`, if not set, then try with `metadata_location` from response. If neither is set, we now throw an error.
--
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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89
--
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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89#discussion_r1378335162
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -109,50 +110,139 @@ pub struct TableMetadata {
}
impl TableMetadata {
+ /// Returns format version of this metadata.
+ #[inline]
+ pub fn format_version(&self) -> FormatVersion {
+ self.format_version.clone()
+ }
+
+ /// Returns uuid of current table.
+ #[inline]
+ pub fn uuid(&self) -> &Uuid {
+ &self.table_uuid
+ }
+
+ /// Returns table location.
+ #[inline]
+ pub fn location(&self) -> &str {
+ self.location.as_str()
+ }
+
+ /// Returns last sequence number.
+ #[inline]
+ pub fn last_sequence_number(&self) -> i64 {
+ self.last_sequence_number
+ }
+
+ /// Returns last updated time.
+ #[inline]
+ pub fn last_updated_ms(&self) -> i64 {
Review Comment:
Tracked in #90
--
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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89#discussion_r1378337379
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -109,50 +110,139 @@ pub struct TableMetadata {
}
impl TableMetadata {
+ /// Returns format version of this metadata.
+ #[inline]
+ pub fn format_version(&self) -> FormatVersion {
+ self.format_version.clone()
+ }
+
+ /// Returns uuid of current table.
+ #[inline]
+ pub fn uuid(&self) -> &Uuid {
+ &self.table_uuid
+ }
+
+ /// Returns table location.
+ #[inline]
+ pub fn location(&self) -> &str {
+ self.location.as_str()
+ }
+
+ /// Returns last sequence number.
+ #[inline]
+ pub fn last_sequence_number(&self) -> i64 {
+ self.last_sequence_number
+ }
+
+ /// Returns last updated time.
+ #[inline]
+ pub fn last_updated_ms(&self) -> i64 {
+ self.last_updated_ms
+ }
+
+ /// Returns schemas
+ #[inline]
+ pub fn schemas(&self) -> impl Iterator<Item = &SchemaRef> {
+ self.schemas.values()
+ }
+
+ /// Lookup schema by id.
+ #[inline]
+ pub fn schema_by_id(&self, schema_id: i32) -> Option<&SchemaRef> {
+ self.schemas.get(&schema_id)
+ }
+
/// Get current schema
#[inline]
- pub fn current_schema(&self) -> Result<Arc<Schema>, Error> {
- self.schemas
- .get(&self.current_schema_id)
- .ok_or_else(|| {
- Error::new(
- ErrorKind::DataInvalid,
- format!("Schema id {} not found!", self.current_schema_id),
- )
- })
- .cloned()
+ pub fn current_schema(&self) -> &SchemaRef {
+ self.schema_by_id(self.current_schema_id)
+ .expect("Current schema id set, but not found in table metadata")
+ }
+
+ /// Returns all partition specs.
+ #[inline]
+ pub fn partition_specs(&self) -> impl Iterator<Item = &PartitionSpecRef> {
+ self.partition_specs.values()
}
+
+ /// Lookup partition spec by id.
+ #[inline]
+ pub fn partition_spec_by_id(&self, spec_id: i32) -> Option<&PartitionSpecRef> {
+ self.partition_specs.get(&spec_id)
+ }
+
/// Get default partition spec
#[inline]
- pub fn default_partition_spec(&self) -> Result<&PartitionSpec, Error> {
- self.partition_specs
- .get(&self.default_spec_id)
- .ok_or_else(|| {
- Error::new(
- ErrorKind::DataInvalid,
- format!("Partition spec id {} not found!", self.default_spec_id),
- )
- })
+ pub fn default_partition_spec(&self) -> Option<&PartitionSpecRef> {
+ if self.default_spec_id == DEFAULT_SPEC_ID {
+ self.partition_spec_by_id(DEFAULT_SPEC_ID)
+ } else {
+ Some(
+ self.partition_spec_by_id(DEFAULT_SPEC_ID)
+ .expect("Default partition spec id set, but not found in table metadata"),
+ )
+ }
+ }
+
+ /// Returns all snapshots
+ #[inline]
+ pub fn snapshots(&self) -> impl Iterator<Item = &SnapshotRef> {
Review Comment:
Collect to `Vec` involves memory allocation, and it's not difficult to do it in rust, just one more line of code. I can thing of some use case where `Vec` is unnecessary:
1. User just wants to print snapshot for debugging.
2. User just wants to search a snapshot later than some creation time in time travel use case.
I think renaming it to `snapshots_iter` is a good suggestion.
--
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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89#discussion_r1378334579
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -109,50 +110,139 @@ pub struct TableMetadata {
}
impl TableMetadata {
+ /// Returns format version of this metadata.
+ #[inline]
+ pub fn format_version(&self) -> FormatVersion {
+ self.format_version.clone()
+ }
+
+ /// Returns uuid of current table.
+ #[inline]
+ pub fn uuid(&self) -> &Uuid {
+ &self.table_uuid
+ }
+
+ /// Returns table location.
+ #[inline]
+ pub fn location(&self) -> &str {
+ self.location.as_str()
+ }
+
+ /// Returns last sequence number.
+ #[inline]
+ pub fn last_sequence_number(&self) -> i64 {
+ self.last_sequence_number
+ }
+
+ /// Returns last updated time.
+ #[inline]
+ pub fn last_updated_ms(&self) -> i64 {
Review Comment:
Good suggestion, but I want to do it in following pr.
--
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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89#discussion_r1378534267
##########
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:
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?
--
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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
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: 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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89#discussion_r1386222676
##########
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:
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 👍
##########
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:
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 👍
--
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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89#discussion_r1378325205
##########
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:
> Take s3 as default seems not cool
How about throwing an error?
> how could we support azblob, gcs and so on
It's not huge effort, we just need to add corresponding configuration in `FileIO`
--
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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89#discussion_r1378335610
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -109,50 +110,139 @@ pub struct TableMetadata {
}
impl TableMetadata {
+ /// Returns format version of this metadata.
+ #[inline]
+ pub fn format_version(&self) -> FormatVersion {
+ self.format_version.clone()
+ }
+
+ /// Returns uuid of current table.
+ #[inline]
+ pub fn uuid(&self) -> &Uuid {
+ &self.table_uuid
+ }
+
+ /// Returns table location.
+ #[inline]
+ pub fn location(&self) -> &str {
+ self.location.as_str()
+ }
+
+ /// Returns last sequence number.
+ #[inline]
+ pub fn last_sequence_number(&self) -> i64 {
+ self.last_sequence_number
+ }
+
+ /// Returns last updated time.
+ #[inline]
+ pub fn last_updated_ms(&self) -> i64 {
+ self.last_updated_ms
+ }
+
+ /// Returns schemas
+ #[inline]
+ pub fn schemas(&self) -> impl Iterator<Item = &SchemaRef> {
+ self.schemas.values()
+ }
+
+ /// Lookup schema by id.
+ #[inline]
+ pub fn schema_by_id(&self, schema_id: i32) -> Option<&SchemaRef> {
Review Comment:
`Arc` is not as cheap as `Rc`, I still prefer to use `&Arc` to avoid unnecessary overhead, and leave it to user to do it.
--
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: [PR] feat: Implement load table api. [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #89:
URL: https://github.com/apache/iceberg-rust/pull/89#discussion_r1378523841
##########
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:
I think it's already kept in `TableMetadata`?
--
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