Discussion: The design of in memory model of iceberg spec.

[liurenjie1024]

When designing in memory model for iceberg spce(metadata, snapshots, manifests etc), there are some problems to consider:

API safety.

For a field in spec, there are several cases to consider:

1. Exists in both v1 and v2, and both are required
2. Exists in both v1 and v2, and both are optional
3. Exists in both v1 and v2, but optional in one and require in another.
4. Exists in v1, but deprecated v2.
5. Exists in v2, and have default value in v1.

Let's use the summary field in snapshot spec as an example. There are some potential solutions to this:

1. Just like in https://github.com/JanKaul/iceberg-rust, we have different models for v1 and v2 spec, e.g. SnapshotV1 and SnapshotV2. This way we can guarantee the api safety of model, but it introduces extra maintaince burden when spec evolves, since most fields are common in both spec.
2. Use one struct for both version(https://github.com/icelake-io/icelake/), and let users to do the check at runtime. This way we don't have extra maintaince effort for different specs, but we can't guarantee the api safety.

Field access

Currently, in both two repos, all fields are accessed through the public field. Personally, I'm not in favor of this approach since it may cause some problems:

1. Misuse of fields. For example, for TableMetadata, when we want to append a new snapshot, we should both append snapshot and snapshot log, but exposing Vec<Snapshot> may lead to wrong usage.
2. In-memory data structure should be designed for high-performance access. For example, TableMetadata should contain a map of snapshot id to snapshot, rather than a vector of snapshots, so that we can access snapshot by id fast.
3. The in-memory data struct fields should not have a one-one mapping as spec, but should provide more friendly access methods to user. For example, there should no ManifestList data structure, and user should access manifest fiels from Snapshot's method.

So for in-memory data structure, I would propose hiding all public fields and provide public methods to access necessary fields.

[Xuanwo]

So for in-memory data structure, I would propose hiding all public fields and provide public methods to access necessary fields.

Agreed.

[Xuanwo]

2. This way we don't have extra maintaince effort for different specs, but we can't guarantee the api safety.

Please correct me if my understanding is wrong: it's safe to convert all v1 table to v2 table during reading. And we only need to care about writing.

[JanKaul]

I think the solution that @Xuanwo is hinting at could be really cool. We could store the data internally similar to the v2 metadata and we only need to care about writing of v1 metadata. If I understood @Fokko right, he implemented it similarly in pyiceberg.

Regarding the field access, I agree with all your points.

[liurenjie1024]

I agree the reading/writing part, but has little concern about the api. Still the summary example(optional in v1, required in v2), what's the return type of Snapshot::summary()?

1. Just Summary struct. Then what should be the value filled for v1 format?
2. Option<Summary>. Then we have to force user to unwrap it when using v2 format.

[liurenjie1024]

Or should we provide a method to return Result<Summary> when so that v2 format user can return an error when there is no Summary?

[JanKaul]

Regardless of the implementation we have the problem that there is a summary in v2 and there is none in v1. So if we want to support v1 we have to use Option/Result. I would prefer Option because I don't see any drawbacks compared to Result, the user has to "unwrap" in either case.

[JanKaul]

Another issue, that @liurenjie1024 pointed out in #2, is that Avro requires a strict schema when reading v1 and v2 values. So we might need two representations for v1 and v2 anyway.

[ZENOTME]

we might need two representations for v1 and v2 anyway

We could split the representation for all structs that have user interaction (i.e. TableMetadata) but use a single representation for the smaller building blocks (i.e. types, values, ..)

Does this means that we need two on-disk representation, but we only need one unique in-memory representation.

[liurenjie1024]

Another issue, that @liurenjie1024 pointed out in #2, is that Avro requires a strict schema when reading v1 and v2 values. So we might need two representations for v1 and v2 anyway.

In fact, we can still have one in memory struct representation, and have two serialize newtypes, like this:

struct TableMetadata {}

struct TableMetadataSerializeV1(TableMetadata);

impl Serialize for TableMetadataSerializeV1 {
 // v1 serialization
}

struct TableMetadataSerializeV2(TableMetadata);

impl Serialize for TableMetadataSerializeV2 {
 // v2 serialization
}

[JanKaul]

I just found a serde feature which might simplify the serialization/deserialization for in-memory/on-disk types. Serde let's you you define a "from" type which specifies that the annotated type first get's deserialized into the "from" type and then converted using the From implementation. And it works similarly with an "into" type. This would allow something like the following:

/// In memory type
#[serde(from = "TableMetadataUnion", into = "TableMetadataUnion")]
struct TableMetadata {}

impl From<TableMetadataUnion> for TableMetadata {}

impl Into<TableMetadataUnion> for TableMetadata {}

/// on disk types
enum TableMetadataUnion {
   V1(TableMetadataV1),
   V2(TableMetadataV2)
}

#[derive(Serialize, Deserialize)]
struct TableMetadataV1 {}

#[derive(Serialize, Deserialize)]
struct TableMetadataV2 {}

What do you think?

[liurenjie1024]

impl Into for TableMetadata {}

How to implement this? I guess that we can depend on a field for table format version? So we need to maintain the same field for all other structs such as snapshot, manifest?

Generally LGTM

[JanKaul]

You are right, we probably need a field for the format version in the TableMetadata struct.

I don't think we need a field in every other struct. When we serialize the TableMetadata we know which format version it is based on the aforementioned field. We can then do the conversion for snapshots, schema,... ourselves inside the impl Into<TableMetadataUnion> for TableMetadata.

I think you would only need a format version field for every struct that you explicitly serialize/deserialize and that is not part of another struct. These could be TableMetadata, ManifestList, ManifestEntry.