sdf-jkl commented on code in PR #10545:
URL: https://github.com/apache/arrow-rs/pull/10545#discussion_r3723532977
##########
parquet/src/arrow/arrow_writer/mod.rs:
##########
@@ -3175,101 +3179,138 @@ mod tests {
})
}
- struct RoundTripOptions {
+ /// Round trip testing fixture:
+ ///
+ /// Tests based on this fixture write data to parquet and then read it
back.
+ struct RoundTripTest {
values: ArrayRef,
- schema: SchemaRef,
+ /// Optionally supplied schema
+ schema: Option<SchemaRef>,
+ /// If the created schema should be nullable. Defaults to true. Ignored
+ /// if schema is set to Some.
+ nullable: bool,
bloom_filter: bool,
bloom_filter_ndv: Option<u64>,
bloom_filter_position: BloomFilterPosition,
}
- impl RoundTripOptions {
- fn new(values: ArrayRef, nullable: bool) -> Self {
- let data_type = values.data_type().clone();
- let schema = Schema::new(vec![Field::new("col", data_type,
nullable)]);
+ impl RoundTripTest {
+ /// Create a test for round tripping values with a nullable schema
+ fn new(values: ArrayRef) -> Self {
Self {
values,
- schema: Arc::new(schema),
+ schema: None,
+ nullable: true,
bloom_filter: false,
bloom_filter_ndv: None,
bloom_filter_position: BloomFilterPosition::AfterRowGroup,
}
}
- }
- fn one_column_roundtrip(values: ArrayRef, nullable: bool) -> Vec<Bytes> {
- one_column_roundtrip_with_options(RoundTripOptions::new(values,
nullable))
- }
+ /// Set the schema
+ fn with_schema(mut self, schema: SchemaRef) -> Self {
+ self.schema = Some(schema);
+ self
+ }
- fn one_column_roundtrip_with_schema(values: ArrayRef, schema: SchemaRef)
-> Vec<Bytes> {
- let mut options = RoundTripOptions::new(values, false);
- options.schema = schema;
- one_column_roundtrip_with_options(options)
- }
+ /// Set the nullable flag
+ fn with_nullable(mut self, nullable: bool) -> Self {
+ self.nullable = nullable;
+ self
+ }
Review Comment:
Or if we keep behavior where `new()` generates a schema, we can go back to
using an existing one -
```suggestion
fn with_non_nullable_schema(mut self) -> Self {
let field =
self.schema.field(0).clone().with_nullable(false);
self.schema = Arc::new(Schema::new_with_metadata(
vec![field],
self.schema.metadata().clone(),
));
self
}
```
##########
parquet/src/arrow/arrow_writer/mod.rs:
##########
@@ -3175,101 +3179,138 @@ mod tests {
})
}
- struct RoundTripOptions {
+ /// Round trip testing fixture:
+ ///
+ /// Tests based on this fixture write data to parquet and then read it
back.
+ struct RoundTripTest {
values: ArrayRef,
- schema: SchemaRef,
+ /// Optionally supplied schema
+ schema: Option<SchemaRef>,
+ /// If the created schema should be nullable. Defaults to true. Ignored
+ /// if schema is set to Some.
+ nullable: bool,
bloom_filter: bool,
bloom_filter_ndv: Option<u64>,
bloom_filter_position: BloomFilterPosition,
}
- impl RoundTripOptions {
- fn new(values: ArrayRef, nullable: bool) -> Self {
- let data_type = values.data_type().clone();
- let schema = Schema::new(vec![Field::new("col", data_type,
nullable)]);
+ impl RoundTripTest {
+ /// Create a test for round tripping values with a nullable schema
+ fn new(values: ArrayRef) -> Self {
Self {
values,
- schema: Arc::new(schema),
+ schema: None,
+ nullable: true,
bloom_filter: false,
bloom_filter_ndv: None,
bloom_filter_position: BloomFilterPosition::AfterRowGroup,
}
}
- }
- fn one_column_roundtrip(values: ArrayRef, nullable: bool) -> Vec<Bytes> {
- one_column_roundtrip_with_options(RoundTripOptions::new(values,
nullable))
- }
+ /// Set the schema
+ fn with_schema(mut self, schema: SchemaRef) -> Self {
+ self.schema = Some(schema);
+ self
+ }
- fn one_column_roundtrip_with_schema(values: ArrayRef, schema: SchemaRef)
-> Vec<Bytes> {
- let mut options = RoundTripOptions::new(values, false);
- options.schema = schema;
- one_column_roundtrip_with_options(options)
- }
+ /// Set the nullable flag
+ fn with_nullable(mut self, nullable: bool) -> Self {
+ self.nullable = nullable;
+ self
+ }
Review Comment:
But I guess it also follows the `Field::with_nullable(bool)` API
##########
parquet/src/arrow/arrow_writer/mod.rs:
##########
@@ -3175,101 +3179,138 @@ mod tests {
})
}
- struct RoundTripOptions {
+ /// Round trip testing fixture:
+ ///
+ /// Tests based on this fixture write data to parquet and then read it
back.
+ struct RoundTripTest {
values: ArrayRef,
- schema: SchemaRef,
+ /// Optionally supplied schema
+ schema: Option<SchemaRef>,
+ /// If the created schema should be nullable. Defaults to true. Ignored
+ /// if schema is set to Some.
+ nullable: bool,
bloom_filter: bool,
bloom_filter_ndv: Option<u64>,
bloom_filter_position: BloomFilterPosition,
}
- impl RoundTripOptions {
- fn new(values: ArrayRef, nullable: bool) -> Self {
- let data_type = values.data_type().clone();
- let schema = Schema::new(vec![Field::new("col", data_type,
nullable)]);
+ impl RoundTripTest {
+ /// Create a test for round tripping values with a nullable schema
+ fn new(values: ArrayRef) -> Self {
Self {
values,
- schema: Arc::new(schema),
+ schema: None,
+ nullable: true,
bloom_filter: false,
bloom_filter_ndv: None,
bloom_filter_position: BloomFilterPosition::AfterRowGroup,
}
}
- }
- fn one_column_roundtrip(values: ArrayRef, nullable: bool) -> Vec<Bytes> {
- one_column_roundtrip_with_options(RoundTripOptions::new(values,
nullable))
- }
+ /// Set the schema
+ fn with_schema(mut self, schema: SchemaRef) -> Self {
+ self.schema = Some(schema);
+ self
+ }
- fn one_column_roundtrip_with_schema(values: ArrayRef, schema: SchemaRef)
-> Vec<Bytes> {
- let mut options = RoundTripOptions::new(values, false);
- options.schema = schema;
- one_column_roundtrip_with_options(options)
- }
+ /// Set the nullable flag
+ fn with_nullable(mut self, nullable: bool) -> Self {
+ self.nullable = nullable;
+ self
+ }
Review Comment:
If schema nullability is the default and `with_nullable(true)` is never
used, can simplify to -
```suggestion
fn with_non_nullable_schema(mut self) -> Self {
self.nullable = false;
self
}
```
--
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]
