dannycjones commented on code in PR #2937:
URL: https://github.com/apache/iceberg-rust/pull/2937#discussion_r3704231908
##########
crates/iceberg/src/runtime/mod.rs:
##########
Review Comment:
I think there's something wrong with `JoinHandle` here. Maybe we should be
exporting it rather than removing the references?
What do you think, @CTTY?
##########
crates/iceberg/src/spec/schema/mod.rs:
##########
@@ -364,25 +364,25 @@ impl Schema {
.and_then(|id| self.field_by_id(*id))
}
- /// Returns [`highest_field_id`].
+ /// Returns the `highest_field_id`.
#[inline]
pub fn highest_field_id(&self) -> i32 {
self.highest_field_id
}
Review Comment:
nitpick: I'd rather we describe what this method is returning, rather than
referring to internal struct details. It's the reason really why the lint is
failing in the first place. In the future, we might not even store this field
and instead compute it on-demand.
i.e.
```rust
/// Returns the highest field ID assigned in this schema.
#[inline]
pub fn highest_field_id(&self) -> i32 {
self.highest_field_id
}
```
The reason for this field is so we can quickly discover the highest field
ID, and then start allocating new ones during schema evolution.
##########
crates/iceberg/src/transaction/update_schema.rs:
##########
@@ -32,12 +32,11 @@ use crate::{Error, ErrorKind, Result, TableRequirement,
TableUpdate};
// Default ID for a new column. This will be re-assigned to a fresh ID at
commit time.
const DEFAULT_FIELD_ID: i32 = 0;
-/// Declarative specification for adding a column in [`UpdateSchemaAction`].
+/// Declarative specification for adding a column in an `UpdateSchemaAction`.
///
/// Use helper constructors such as [`AddColumn::optional`] and
[`AddColumn::required`],
-/// optionally combined with [`AddColumn::with_parent`] and
[`AddColumn::with_doc`], then pass
-/// the value to
-/// [`UpdateSchemaAction::add_column`].
+/// optionally combined with the builder's `parent` and `doc` setters via
+/// [`AddColumn::builder`], then pass the value to
`UpdateSchemaAction::add_column`.
Review Comment:
This is another place that I think we've got an issue with - we should
likely be exporting `UpdateSchemaAction` properly.
We shouldn't allow library consumers to construct `UpdateSchemaAction`
(that's already `pub(crate)` other than via a transaction, but we should export
the type so that it's documented.
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -1605,7 +1605,7 @@ pub struct SnapshotLog {
}
impl SnapshotLog {
- /// Returns the last updated timestamp as a DateTime<Utc> with millisecond
precision
+ /// Returns the last updated timestamp as a `DateTime<Utc>` with
millisecond precision
Review Comment:
This is one we can and should link to!
--
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]
