Remove unwrap() in ManifestListWriter.close()

[zeodtr]

unwrap() should not be used in normal code, but ManifestListWriter.close() uses it.
It should be removed and the error should be properly handled like ManifestWriter.write().

[liurenjie1024]

Thanks for reporting!

[zeodtr]

@Fokko @liurenjie1024 I think this issue has not been resolved, since I'm kind of disagree with the pull request.
What I'm concerned about are as follows:

• expect() also should not be used since it ends the program with panic just like unwrap(). In a production program that's unacceptable, especially for a server program.
• It might not be enough to just replace unwrap() with ? in ManifestListWriter's write-related codes. In ManifestWriter, it also adds an appropriate error message. Please see:

  iceberg-rust/crates/iceberg/src/spec/manifest.rs

  Line 295 in 9ae9e13

  writer.write_all(&content).await.map_err(|err| {

Could you please reopen this issue?

[liurenjie1024]

@zeodtr Sorry for the mistake, yes you are right, expect still may cause panic. Let's reopen it. cc @odysa We may need to modify the From to TryFrom.

[odysa]

We may need to modify the From to TryFrom.

serde does not support the attribute try_into. serde-rs/serde#1524 The into attribute requires From. Converting to TryFrom requires us to manually implement a serializer.

iceberg-rust/crates/iceberg/src/spec/table_metadata.rs

Line 48 in 9ae9e13

#[serde(try_from = "TableMetadataEnum", into = "TableMetadataEnum")]

[liurenjie1024]

I remember why we use expect for TableMetadata serialization, see this comment. But I still agree that it would be best not to panic in a serious lib which may crash programs.

serde does not support the attribute try_into. serde-rs/serde#1524 The into attribute requires From. Converting to TryFrom requires us to manually implement a serializer.

Yes, it seems we still need to implement Serialization/Deserialization manually, but it's still a wrapper of the conversion from TableMetadataEnum.

[zeodtr]

In the following code, from:

iceberg-rust/crates/iceberg/src/spec/table_metadata.rs

Line 679 in 9ae9e13

impl From<TableMetadata> for TableMetadataV1 {

    impl From<TableMetadata> for TableMetadataV1 {
        fn from(v: TableMetadata) -> Self {
            TableMetadataV1 {
                format_version: VersionNumber::<1>,
                table_uuid: Some(v.table_uuid),
                location: v.location,
                last_updated_ms: v.last_updated_ms,
                last_column_id: v.last_column_id,
                schema: v
                    .schemas
                    .get(&v.current_schema_id)
                    .expect("current_schema_id not found in schemas")
                    .as_ref()
                    .clone()
                    .into(),

The code expect()s current_schema_id should not be None. But it is valid to be None in TableMetadataV1, although such usage is deprecated. (See Iceberg spec https://iceberg.apache.org/spec/#table-metadata-fields) I think that using expect() is unacceptable in such a case. Actually, in this case, I think that the code should instead use the schema field of TableMetadataV1 when the current_schema_id field is None.

[odysa]

The code expect()s current_schema_id should not be None

current_schema_id is impossible to be None in TableMetadata

But it is valid to be None in TableMetadataV1

It's handled here.

iceberg-rust/crates/iceberg/src/spec/table_metadata.rs

Line 556 in 9ae9e13

.unwrap_or_else(|| schemas.keys().copied().max().unwrap_or_default()),

code should instead use the schema field of TableMetadataV1 when the current_schema_id field is None

When we convert TableMetadata to TableMetadataV1, we have to look up schemas since there is no schema field in TableMetadata

expect() is unacceptable in such a case

I agree. It panics only when schemas does not contain current_schema_id, which means TableMetadataV1 may be invalid.

[zeodtr]

@odysa Apologies, I have totally misinterpreted the code. I regret any inconvenience caused by my misunderstanding. Please ignore my previous comment.

[tabmatfournier]

Is this issue still valid given #185 ? This seems addressed. Apologies for the noise, surfing good first issues for something to get started on.

[liurenjie1024]

Is this issue still valid given #185 ? This seems addressed. Apologies for the noise, surfing good first issues for something to get started on.

@tabmatfournier Yeah, I think it's closed, thanks for reporting this.