Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple building and serialization #778

Open
Sl1mb0 opened this issue Dec 11, 2024 · 4 comments
Open

Decouple building and serialization #778

Sl1mb0 opened this issue Dec 11, 2024 · 4 comments

Comments

@Sl1mb0
Copy link
Contributor

Sl1mb0 commented Dec 11, 2024

At the moment, the building and serialization of Iceberg metadata is coupled together.

For example, let's say I want to build a ManifestFile that I then add to a ManifestList:

(some code has not been included for the sake of brevity)

let manifest_file_path = NamedTempFile::new().unwrap();
let manifest_file_output = FileIOBuilder::new_fs_io()
    .build()
    .unwrap()
    .new_output(manifest_file_path.path().to_str().unwrap())
    .unwrap();


let manifest_writer = ManifestWriter::new(manifest_file_output, 0, Vec::new());

let manifest_file = manifest_writer
    .write(manifest)
    .await
    .unwrap()
    
let manifest_list_path = NamedTempFile::new().unwrap();
let manifest_list_output = FileIOBuilder::new_fs_io()
    .build()
    .unwrap()
    .new_output(manifest_list_path.path().to_str().unwrap())
    .unwrap();

let mut writer = ManifestListWriter::v2(manifest_list_output,0,0,0);

writer.add_manifests(vec![manifest_file]);

writer.close().await.unwrap();
  • There is an abstract coupling of building and serialization: in order to 'build' a ManifestFile you have to 'write' a Manifest.
  • There is another abstract coupling of building/serde: The where this metadata gets written to is included in the what metadata is written
    • When you specify a location to write a ManifestFile to - that location is where the ManifestFile gets written to and is included in the metadata of that ManifestFile
    • This means that when the built ManifestFile is added to a ManifestList, the location of the ManifestFile is what's used to 'point' the ManifestList to that ManifestFile
  • This coupling forces the user to use the FileIO/OutputFile/InputFile type to write to their preferred storage layer instead of allowing the user to build/use their own abstractions for "where the bytes get written to"
    • We would really like to separate the building and serialization layers as that will allow us to use our own storage layer abstractions.
    • To provide an example: if the user wants to use their own storage layer for storing metadata bytes
      • They must build/write all the necessary metadata types using FileIO
      • They would then need to 'copy' all these bytes to their preferred storage layer
      • ⚠️ problem ⚠️
        • Because the metadata itself contains "where" the metadata is once that metadata is "moved" somewhere else, it's no longer valid. This is because the 'metadata hierarchy' (IE which metadata points to which snapshot points to which manifest list etc) is only valid for where it was built/serialized. To illustrate this:

image

In the above example the ManifestList and ManifestFile were built/serialized on Node A and then copied over to Node B but because the building/serialization was performed on Node A - the ManifestList on Node B points to the ManifestFile on Node A. _this happens because the location of where the ManifestFile was written to was included in it's metadata; and that metadata informs the ManifestList where that ManifestFile is.

Sl1mb0 added a commit to influxdata/iceberg-rust that referenced this issue Dec 11, 2024
This commit changes a couple function signatures that effectively allow the user to differentiate where metadata bytes get written to
and what's contained in the metadata type. For more info on why see: <apache#778>
@Fokko
Copy link
Contributor

Fokko commented Dec 12, 2024

Hey @Sl1mb0, Thanks for raising this issue. If I understand correctly if you could provide your own implementation of FileIO, would you be able to make it work? This would avoid the copy.

Iceberg works with absolute paths that are immutable after being written. This is very important for merge-on-read operations such as positional deletes.

@Sl1mb0
Copy link
Contributor Author

Sl1mb0 commented Dec 12, 2024

If I understand correctly if you could provide your own implementation of FileIO, would you be able to make it work? This would avoid the copy.

Hmm - this may work, but it's hard to say given it's not clear what the FileIO trait would look like at the moment.

Iceberg works with absolute paths that are immutable after being written.

I understand that, but in my opinion that is the responsibility of a 'higher-level' layer above building and serialization. Ideally you would have *Builders for each type of metadata: ManifestFile, ManifestList, etc. You could then implement ToBytes for each type, and that would handle the serialization of those types. This would also allow users to either use FileIO out of the box or provide their own implementations for "where the bytes go". I think you would need to be very clear with documentation about the invariants that users must uphold. For example, the invariant: where a ManifestFile is written to must match the manifest_path for that ManifestFile.

@Sl1mb0
Copy link
Contributor Author

Sl1mb0 commented Dec 13, 2024

I should clarify: I would be happy do this work at some point but would like to know if it would be accepted.

@liurenjie1024
Copy link
Contributor

liurenjie1024 commented Dec 19, 2024

Hi, @Sl1mb0 Thanks for raising this! If I understand correctly, you are asking to parts:

  1. A builder method for ManifestFile/ManifestList, instead of building them using writers only?
  2. Exposing serialization/deserialization method of these structs like [this one]?

So that user can control where to store these metadata?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants