-
Notifications
You must be signed in to change notification settings - Fork 359
refactor(writer): Make writer builders non-consuming in build #1889
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
base: main
Are you sure you want to change the base?
Changes from all commits
96f654a
029a384
db891df
6b3dac1
967c155
738fafe
b58c463
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ use crate::Result; | |
| use crate::spec::{DataFileFormat, PartitionKey, TableMetadata}; | ||
|
|
||
| /// `LocationGenerator` used to generate the location of data file. | ||
| pub trait LocationGenerator: Clone + Send + 'static { | ||
| pub trait LocationGenerator: Clone + Send + Sync + 'static { | ||
| /// Generate an absolute path for the given file name that includes the partition path. | ||
| /// | ||
| /// # Arguments | ||
|
|
@@ -94,7 +94,7 @@ impl LocationGenerator for DefaultLocationGenerator { | |
| } | ||
|
|
||
| /// `FileNameGeneratorTrait` used to generate file name for data file. The file name can be passed to `LocationGenerator` to generate the location of the file. | ||
| pub trait FileNameGenerator: Clone + Send + 'static { | ||
| pub trait FileNameGenerator: Clone + Send + Sync + 'static { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned above. |
||
| /// Generate a file name. | ||
| fn generate_file_name(&self) -> String; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -36,11 +36,11 @@ pub mod rolling_writer; | |||||||||||||||||||||||
| type DefaultOutput = Vec<DataFileBuilder>; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// File writer builder trait. | ||||||||||||||||||||||||
| pub trait FileWriterBuilder<O = DefaultOutput>: Send + Clone + 'static { | ||||||||||||||||||||||||
| pub trait FileWriterBuilder<O = DefaultOutput>: Clone + Send + Sync + 'static { | ||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we need to keep this
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the
For reference #1735 (comment). iceberg-rust/crates/iceberg/src/writer/file_writer/rolling_writer.rs Lines 106 to 116 in b58c463
|
||||||||||||||||||||||||
| /// 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) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above.