Encapsulate parquet round trip test logic in struct - #10545
Conversation
|
|
||
| one_column_roundtrip(Arc::clone(&string_view_values), false); | ||
| one_column_roundtrip(Arc::clone(&binary_view_values), false); | ||
| RoundTripTest::new(Arc::clone(&string_view_values)) |
There was a problem hiding this comment.
I think this is much easier to understand now -- the nullable_flag is now self documenting
| /// Round trip testing fixture: | ||
| /// | ||
| /// Tests based on this fixture write data to parquet and then read it back. | ||
| struct RoundTripTest { |
There was a problem hiding this comment.
I renamed and moved a few parameters to fields
| builder = builder.set_bloom_filter_max_ndv(ndv); | ||
| } | ||
| let props = builder.build(); | ||
| /// Run the test specified by the options, returning the encoded Parquet bytes |
There was a problem hiding this comment.
Viewing this diff without whitespace makes this change clearer I think: https://github.com/apache/arrow-rs/pull/10545/changes?w=1
The method is indented but the only new code is this
let schema = schema.unwrap_or_else(|| {
let data_type = values.data_type().clone();
Arc::new(Schema::new(vec![Field::new("col", data_type, nullable)]))
});
…r_ndv Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alamb
changed the title
sdf-jkl
left a comment
sdf-jkl
left a comment
There was a problem hiding this comment.
@alamb A few suggestions, but nothing serious.
- drop the
nullabilityflag and go back to generating the default schema innew() - make
with_nullablealways set nullability to false. Change the name towith_non_nullable_schemaand reuse the existing schema instead of generating a new one.
| fn with_nullable(mut self, nullable: bool) -> Self { | ||
| self.nullable = nullable; | ||
| self | ||
| } |
There was a problem hiding this comment.
If schema nullability is the default and with_nullable(true) is never used, can simplify to -
| fn with_nullable(mut self, nullable: bool) -> Self { | |
| self.nullable = nullable; | |
| self | |
| } | |
| fn with_non_nullable_schema(mut self) -> Self { | |
| self.nullable = false; | |
| self | |
| } |
There was a problem hiding this comment.
Or if we keep behavior where new() generates a schema, we can go back to using an existing one -
| fn with_nullable(mut self, nullable: bool) -> Self { | |
| self.nullable = nullable; | |
| self | |
| } | |
| 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 | |
| } |
There was a problem hiding this comment.
But I guess it also follows the Field::with_nullable(bool) API
There was a problem hiding this comment.
Yeah, if you don't mind I am going to keep following the Field::with_nullable API
I have big hopes to unify these round trip tests with the ones that use record batches as well. We'll see if that is reasonable or not
4 participants
Which issue does this PR close?
Rationale for this change
I am trying to understand the depth of our round trip tests (there are over 4000
lines of tests in the arrow writer module). After some study it appears there
are tests to round trip both single columns and record batches, which share some
non trivial amount of logic.
It also makes it hard to evaluate coverage because there are several similar but not quite the same free functions and it is unclear hwo they are related to each other
and what some of the parameters mean (like the argument to
roundtrip_one_column)I think making it easier to find and evaluate test coverage will make it easier to
maintain and extend this crate in the future.
What changes are included in this PR?
RoundTripTestrather than free functionsAre these changes tested?
Only tests
Are there any user-facing changes?
No