Skip to content

chore(refactor) refactor ManifestWriterBuilder to take Box<dyn FileWrite> and location#2568

Open
xanderbailey wants to merge 2 commits into
apache:mainfrom
xanderbailey:xb/refactor-manifest-writer
Open

chore(refactor) refactor ManifestWriterBuilder to take Box<dyn FileWrite> and location#2568
xanderbailey wants to merge 2 commits into
apache:mainfrom
xanderbailey:xb/refactor-manifest-writer

Conversation

@xanderbailey
Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Working towards: #2034

What changes are included in this PR?

Similar reasons to #2564. For the current encryption work we will construct a FileWrite to do transparent encryption on write, this means we need to change the constructor of the ManifestWriterBuilder to now take both a Box<dyn FileWrite> and a location.

Are these changes tested?

@xanderbailey xanderbailey force-pushed the xb/refactor-manifest-writer branch from b83e6bb to d69e4c5 Compare June 2, 2026 08:39
/// Create a new builder.
pub fn new(
output: OutputFile,
writer: Box<dyn FileWrite>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to this breaking api change, instead, we could add a new method to accept encrypted output file, and these two methods share same function which accepts FileWriter.

Copy link
Copy Markdown
Contributor Author

@xanderbailey xanderbailey Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I quite follow sorry, is that to say we keep this constructor and store something like:

writer: Pin<Box<dyn Future<Output = Result<Box<dyn FileWrite>>> + Send>>

in the struct and have a new constructor:

    pub fn new_from_encrypted(
        encrypted_output: EncryptedOutputFile,
        snapshot_id: Option<i64>,
        key_metadata: Option<Vec<u8>>,
        schema: SchemaRef,
        partition_spec: PartitionSpec,
    ) -> Self {
        let location = encrypted_output.location().to_owned();
        Self {
            writer_future: Box::pin(async move { encrypted_output.writer().await }),
            location,
            snapshot_id,
            key_metadata,
            schema,
            partition_spec,
        }
    }

?

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

Successfully merging this pull request may close these issues.

2 participants