alamb commented on code in PR #2729:
URL: https://github.com/apache/arrow-rs/pull/2729#discussion_r971938352
##########
arrow/src/record_batch.rs:
##########
@@ -412,16 +412,47 @@ pub struct RecordBatchOptions {
/// Optional row count, useful for specifying a row count for a
RecordBatch with no columns
pub row_count: Option<usize>,
}
+/// Builder to create new RecordBatchOptions object
+#[derive(Debug)]
+pub struct RecordBatchOptionsBuilder {
+ /// Match field names of structs and lists. If set to `true`, the names
must match.
+ match_field_names: bool,
-impl Default for RecordBatchOptions {
+ /// Optional row count, useful for specifying a row count for a
RecordBatch with no columns
+ row_count: Option<usize>,
+}
+impl Default for RecordBatchOptionsBuilder {
fn default() -> Self {
+ Self::new()
+ }
+}
+
+impl RecordBatchOptionsBuilder {
+ pub fn new() -> Self {
Self {
match_field_names: true,
row_count: None,
}
}
+ pub fn match_field_names(mut self, match_field_names: bool) -> Self {
Review Comment:
```suggestion
pub fn with_match_field_names(mut self, match_field_names: bool) -> Self
{
```
##########
arrow/src/ipc/reader.rs:
##########
@@ -578,10 +578,9 @@ pub fn read_record_batch(
let mut node_index = 0;
let mut arrays = vec![];
- let options = RecordBatchOptions {
- row_count: Some(batch.length() as usize),
- ..Default::default()
Review Comment:
As @askoa describes, this pattern is not possible outside this crate (e.g.
in DataFusion) as the RecordBatchOptions are marked as non exhaustive.
##########
arrow/src/record_batch.rs:
##########
@@ -412,16 +412,47 @@ pub struct RecordBatchOptions {
/// Optional row count, useful for specifying a row count for a
RecordBatch with no columns
pub row_count: Option<usize>,
}
+/// Builder to create new RecordBatchOptions object
+#[derive(Debug)]
+pub struct RecordBatchOptionsBuilder {
+ /// Match field names of structs and lists. If set to `true`, the names
must match.
+ match_field_names: bool,
-impl Default for RecordBatchOptions {
+ /// Optional row count, useful for specifying a row count for a
RecordBatch with no columns
+ row_count: Option<usize>,
+}
+impl Default for RecordBatchOptionsBuilder {
fn default() -> Self {
+ Self::new()
+ }
+}
+
+impl RecordBatchOptionsBuilder {
+ pub fn new() -> Self {
Self {
match_field_names: true,
row_count: None,
}
}
+ pub fn match_field_names(mut self, match_field_names: bool) -> Self {
+ self.match_field_names = match_field_names;
+ self
+ }
+ pub fn row_count(mut self, row_count: usize) -> Self {
Review Comment:
```suggestion
pub fn row_count(mut self, row_count: Option<usize>) -> Self {
```
I recommend having this parameter take an Option, otherwise there is no way
to set it to `None`.
##########
arrow/src/record_batch.rs:
##########
@@ -412,16 +412,47 @@ pub struct RecordBatchOptions {
/// Optional row count, useful for specifying a row count for a
RecordBatch with no columns
pub row_count: Option<usize>,
}
+/// Builder to create new RecordBatchOptions object
+#[derive(Debug)]
+pub struct RecordBatchOptionsBuilder {
+ /// Match field names of structs and lists. If set to `true`, the names
must match.
+ match_field_names: bool,
-impl Default for RecordBatchOptions {
+ /// Optional row count, useful for specifying a row count for a
RecordBatch with no columns
+ row_count: Option<usize>,
+}
+impl Default for RecordBatchOptionsBuilder {
fn default() -> Self {
+ Self::new()
+ }
+}
+
+impl RecordBatchOptionsBuilder {
+ pub fn new() -> Self {
Self {
match_field_names: true,
row_count: None,
}
}
+ pub fn match_field_names(mut self, match_field_names: bool) -> Self {
Review Comment:
I think the convention in this crate is to use the `with_` prefix for
builder style apis
For example:
https://docs.rs/arrow/22.0.0/arrow/datatypes/struct.Field.html#method.with_name
ALl uses: https://docs.rs/arrow/22.0.0/arrow/?search=with_
though @tustvold may correct me
##########
arrow/src/record_batch.rs:
##########
@@ -412,16 +412,47 @@ pub struct RecordBatchOptions {
/// Optional row count, useful for specifying a row count for a
RecordBatch with no columns
pub row_count: Option<usize>,
}
+/// Builder to create new RecordBatchOptions object
+#[derive(Debug)]
+pub struct RecordBatchOptionsBuilder {
Review Comment:
I can't think if a reason to have a separate builder from the Options
structure.
What do you think about simply making `RecordBatchOptions` *also* be a
builder -- aka put make `RecordBatchOptions::with_row_size` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
