refine: refine writer interface by ZENOTME · Pull Request #741 · apache/iceberg-rust

ZENOTME · 2024-11-29T09:53:51Z

Hi, I'm working on writer recently and find some weaknesses of our original writer interface design:

Our IcebergWriter Builder interface looks like following:

trait IcebergWriterBuilder {
 type C;
 async fn build(self, config: Self::C) -> Result<Self::R>;
}

And I realized that this custom config param cause we can't combine the write flexibility. E.g. In partition writer

struct PartitionWriter<IcebergWriterBuilder> {
  inner_writer_builder: B
}

impl  PartitionWriter<IcebergWriterBuilder>  {
  pub async fn write(..) {
      self.inner_writer_builder.build(...) // We can't build because we don't know pass which param.
  }
}

So avoid this problem, we should pass the custom param when create the builder and the build interface should looks like fn build() -> Self

The schema of FileWriter can determined by base writer like: data file writer, position delete writer, equality delete writer.
In our original design, user should pass the schema to file writer builder when create them like

let file_builder = ParquetWriterBuilder(schema)

However, sometimes the schema is hard to determine when we create them. E.g. equality delete writer, we only know what the schema looks like util we pass the equality id and create the equality delete writer. To avoid the problem, we change fn build() -> Self of FileWriterBuilder to fn build(schema:SchemaRef) -> Self. By this way, the schema of FileWriter is determined by base writer.

I send this discussion as a PR to make it easier to express my idea. BTW, I applied this change and complete partition writer, delta writer in https://github.com/ZENOTME/iceberg-rust/tree/partition_writer and it looks well.

Feel free for any suggestion. cc @liurenjie1024 @Xuanwo @Fokko @c-thiel

liurenjie1024

Thanks @ZENOTME for this pr, left some question about this change.

liurenjie1024 · 2024-12-04T03:32:27Z

crates/iceberg/src/writer/file_writer/mod.rs

    type R: FileWriter<O>;
    /// Build file writer.
-    fn build(self) -> impl Future<Output = Result<Self::R>> + Send;
+    fn build(self, schema: SchemaRef) -> impl Future<Output = Result<Self::R>> + Send;


I don't understand, why this can't be passed as constructor parameter of the FileWriterBuilder?

Let's use original process of creating equality delete writer as example:

let equality_ids = vec![0_i32, 8]; let equality_config = EqualityDeleteWriterConfig::new(equality_ids, Arc::new(schema), None).unwrap(); let delete_schema = arrow_schema_to_schema(equality_config.projected_arrow_schema_ref()).unwrap(); let projector = equality_config.projector.clone(); // prepare writer let pb = ParquetWriterBuilder::new( WriterProperties::builder().build(), Arc::new(delete_schema), file_io.clone(), location_gen, file_name_gen, ); let mut equality_delete_writer = EqualityDeleteFileWriterBuilder::new(pb) .build(equality_config) .await?;

We need to get the projected schema from EqualityDeleteWriterConfig. Otherwise, the user have to project the schema by themselves. After remove writer config, we need to other thing help us to project the schema and let user get from it.
One pattern I realized for now is that the schema of file writer is always rely on the iceberg writer which use it. So I think move it to the build function can make thing looks more simple.🤔

Sorry, but I'm not convinced that this simplified things a lot. I think the construction of writers will not be called directly by user, since they are low level apis. Schema is only part of the arguments.

I'm a beginner rustacean, but you could still pass the equality_config to the constructor, right?

And I agree with @liurenjie1024, I think in general we should think more about the high-level API. For example, constructing the writers by yourself can be pretty error-prone. Much rather I would see a Table object returning a fully configured writer based on the current-schema.

Make sense. Let's remove the change.

liurenjie1024 · 2024-12-04T03:32:57Z

crates/iceberg/src/writer/mod.rs

    /// Get the current file written size.
    fn current_written_size(&self) -> usize;
+    /// Get the schema of the current file.
+    fn current_schema(&self) -> SchemaRef;


When this will be useful?

This function will be useful in delta writer. But it's fine to remove for now, let's add it until we find it's required.

liurenjie1024

Thanks @ZENOTME for this pr, LGTM!

liurenjie1024 · 2024-12-09T10:25:25Z

crates/iceberg/src/writer/mod.rs

 /// The current file status of iceberg writer. It implement for the writer which write a single
 /// file.
-pub trait CurrentFileStatus {
+pub trait CurrentWriterStatus {


It would be better to split to another pr.

Let's remove them.

liurenjie1024 · 2024-12-09T10:28:06Z

Let's wait for a while to see other reviewer's point of view.

c-thiel

LGTM, thanks @ZENOTME.

I think we should derive Debug for most structs in public interfaces, I just commented the first, but there are a few touched by this PR. At least for EqualityDeleteWriterConfig it would be helpful. For EqualityDeleteWriterConfig, or configs in general, PartialEq would also be good.

c-thiel · 2024-12-09T11:55:53Z

crates/iceberg/src/writer/base_writer/data_file_writer.rs

@@ -29,38 +29,27 @@ use crate::Result;
 #[derive(Clone)]
 pub struct DataFileWriterBuilder<B: FileWriterBuilder> {


Should we maybe also derive Debug?

ZENOTME mentioned this pull request Dec 3, 2024

feat: support position delete writer #704

Open

liurenjie1024 reviewed Dec 4, 2024

View reviewed changes

ZENOTME force-pushed the refine_writer_interface branch 5 times, most recently from 738a4a2 to db06454 Compare December 6, 2024 13:51

ZENOTME requested review from Fokko and liurenjie1024 December 6, 2024 14:20

liurenjie1024 approved these changes Dec 9, 2024

View reviewed changes

remove config param in writer builder

d0a2897

ZENOTME force-pushed the refine_writer_interface branch from db06454 to d0a2897 Compare December 9, 2024 10:45

c-thiel reviewed Dec 9, 2024

View reviewed changes

add Debug derive

d51ab85

liurenjie1024 merged commit 8b90c84 into apache:main Dec 10, 2024

ZENOTME mentioned this pull request Dec 10, 2024

refactor: avoid async_trait macro for IcebergWriter and provide extra dyn trait for object safety #760

Closed

		@@ -29,38 +29,27 @@ use crate::Result;
		#[derive(Clone)]
		pub struct DataFileWriterBuilder<B: FileWriterBuilder> {

Conversation

ZENOTME commented Nov 29, 2024

Uh oh!

liurenjie1024 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZENOTME Dec 4, 2024 • edited by Fokko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liurenjie1024 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liurenjie1024 commented Dec 9, 2024

Uh oh!

c-thiel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ZENOTME Dec 4, 2024 •

edited by Fokko

Loading