Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions crates/iceberg/src/writer/base_writer/data_file_writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ where
#[async_trait::async_trait]
impl<B, L, F> IcebergWriterBuilder for DataFileWriterBuilder<B, L, F>
where
B: FileWriterBuilder,
L: LocationGenerator,
F: FileNameGenerator,
B: FileWriterBuilder + Sync,
L: LocationGenerator + Sync,
F: FileNameGenerator + Sync,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we should just add supertrait Sync to these traits to enforce it across different implementations

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.

Also we should remove the Clone in trait implementation in builders.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback, updated in 967c155.

{
type R = DataFileWriter<B, L, F>;

async fn build(self, partition_key: Option<PartitionKey>) -> Result<Self::R> {
async fn build(&self, partition_key: Option<PartitionKey>) -> Result<Self::R> {
Ok(DataFileWriter {
inner: Some(self.inner.clone().build()),
partition_key,
Expand Down
12 changes: 6 additions & 6 deletions crates/iceberg/src/writer/base_writer/equality_delete_writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,17 +117,17 @@ impl EqualityDeleteWriterConfig {
#[async_trait::async_trait]
impl<B, L, F> IcebergWriterBuilder for EqualityDeleteFileWriterBuilder<B, L, F>
where
B: FileWriterBuilder,
L: LocationGenerator,
F: FileNameGenerator,
B: FileWriterBuilder + Sync,
L: LocationGenerator + Sync,
F: FileNameGenerator + Sync,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same here

{
type R = EqualityDeleteFileWriter<B, L, F>;

async fn build(self, partition_key: Option<PartitionKey>) -> Result<Self::R> {
async fn build(&self, partition_key: Option<PartitionKey>) -> Result<Self::R> {
Ok(EqualityDeleteFileWriter {
inner: Some(self.inner.clone().build()),
projector: self.config.projector,
equality_ids: self.config.equality_ids,
projector: self.config.projector.clone(),
equality_ids: self.config.equality_ids.clone(),
partition_key,
})
}
Expand Down
2 changes: 1 addition & 1 deletion crates/iceberg/src/writer/file_writer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pub trait FileWriterBuilder<O = DefaultOutput>: Send + Clone + 'static {
/// The associated file writer type.
type R: FileWriter<O>;
/// Build file writer.
fn build(self, output_file: OutputFile) -> impl Future<Output = Result<Self::R>> + Send;
fn build(&self, output_file: OutputFile) -> impl Future<Output = Result<Self::R>> + Send;
}

/// File writer focus on writing record batch to different physical file format.(Such as parquet. orc)
Expand Down
4 changes: 2 additions & 2 deletions crates/iceberg/src/writer/file_writer/parquet_writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,11 @@ impl ParquetWriterBuilder {
impl FileWriterBuilder for ParquetWriterBuilder {
type R = ParquetWriter;

async fn build(self, output_file: OutputFile) -> Result<Self::R> {
async fn build(&self, output_file: OutputFile) -> Result<Self::R> {
Ok(ParquetWriter {
schema: self.schema.clone(),
inner_writer: None,
writer_properties: self.props,
writer_properties: self.props.clone(),
current_row_num: 0,
output_file,
nan_value_count_visitor: NanValueCountVisitor::new_with_match_mode(self.match_mode),
Expand Down
10 changes: 5 additions & 5 deletions crates/iceberg/src/writer/file_writer/rolling_writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,15 +103,15 @@ where
}

/// Build a new [`RollingFileWriter`].
pub fn build(self) -> RollingFileWriter<B, L, F> {
pub fn build(&self) -> RollingFileWriter<B, L, F> {
RollingFileWriter {
inner: None,
inner_builder: self.inner_builder,
inner_builder: self.inner_builder.clone(),
target_file_size: self.target_file_size,
data_file_builders: vec![],
file_io: self.file_io,
location_generator: self.location_generator,
file_name_generator: self.file_name_generator,
file_io: self.file_io.clone(),
location_generator: self.location_generator.clone(),
file_name_generator: self.file_name_generator.clone(),
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions crates/iceberg/src/writer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,10 @@
//! }
//!
//! #[async_trait::async_trait]
//! impl<B: IcebergWriterBuilder> IcebergWriterBuilder for LatencyRecordWriterBuilder<B> {
//! impl<B: IcebergWriterBuilder + Sync> IcebergWriterBuilder for LatencyRecordWriterBuilder<B> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there any reason why we don't add Sync as supertrait here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@CTTY This was the missing part. Thank you!

//! type R = LatencyRecordWriter<B::R>;
//!
//! async fn build(self, partition_key: Option<PartitionKey>) -> Result<Self::R> {
//! async fn build(&self, partition_key: Option<PartitionKey>) -> Result<Self::R> {
//! Ok(LatencyRecordWriter {
//! inner_writer: self.inner_writer_builder.build(partition_key).await?,
//! })
Expand Down Expand Up @@ -404,7 +404,7 @@ pub trait IcebergWriterBuilder<I = DefaultInput, O = DefaultOutput>:
/// The associated writer type.
type R: IcebergWriter<I, O>;
/// Build the iceberg writer with an optional partition key.
async fn build(self, partition_key: Option<PartitionKey>) -> Result<Self::R>;
async fn build(&self, partition_key: Option<PartitionKey>) -> Result<Self::R>;
}

/// The iceberg writer used to write data to iceberg table.
Expand Down
Loading