alamb commented on code in PR #10585:
URL: https://github.com/apache/arrow-rs/pull/10585#discussion_r3736519005
##########
parquet/src/arrow/arrow_writer/mod.rs:
##########
@@ -2467,22 +2450,15 @@ mod tests {
let string_values = StringArray::from(raw_string_values.clone());
let binary_values = BinaryArray::from(raw_binary_value_refs);
- let batch = RecordBatch::try_new(
- Arc::new(schema),
- vec![Arc::new(string_values), Arc::new(binary_values)],
- )
- .unwrap();
+ assert_eq!(string_values.null_count(), 0);
+ assert_eq!(binary_values.null_count(), 0);
- roundtrip(batch, Some(SMALL_SIZE / 2));
+ RoundTripTest::new(Arc::new(string_values)).run();
Review Comment:
technically speaking this encodes a single column batch rather than a
multi-column batch, but the point of these tests is to test the round tripping
of data through the whole parquet machiner; Batches with multiple columns are
tested elsewhere
##########
parquet/src/arrow/arrow_writer/mod.rs:
##########
@@ -2533,21 +2497,6 @@ mod tests {
RoundTripTest::new(Arc::clone(&binary_view_values))
.with_nullable(false)
.run();
-
- let batch = RecordBatch::try_new(
- Arc::new(schema),
- vec![string_view_values, binary_view_values],
- )
- .unwrap();
-
- // Disable dictionary to exercise plain encoding paths in the reader.
- for version in [WriterVersion::PARQUET_1_0,
WriterVersion::PARQUET_2_0] {
Review Comment:
RoundtripTest also tests with/without dictionary and data page formats (and
several other parameters)
https://github.com/apache/arrow-rs/blob/725fe70db6efcd45bbdbdd6f3ef97c8c71221fad/parquet/src/arrow/arrow_writer/mod.rs#L3199-L3198
##########
parquet/src/arrow/arrow_writer/mod.rs:
##########
@@ -2221,16 +2221,12 @@ mod tests {
#[test]
fn arrow_writer_non_null() {
- // define schema
let schema = Schema::new(vec![Field::new("a", DataType::Int32,
false)]);
-
- // create some data
let a = Int32Array::from(vec![1, 2, 3, 4, 5]);
- // build a record batch
- let batch = RecordBatch::try_new(Arc::new(schema),
vec![Arc::new(a)]).unwrap();
-
- roundtrip(batch, Some(SMALL_SIZE / 2));
+ RoundTripTest::new(Arc::new(a))
+ .with_schema(Arc::new(schema))
+ .run();
Review Comment:
This actually increases test coverage as
`RoundTripTest::run()` actually already calls into the same `roundtrip`
code, but does so with:
1. Multiple different row group sizes (including SMALL_SIZE/2)
2. All supported encodings
3. Dictionary/no dictionary pages
4. DataPage V1 and V2
https://github.com/apache/arrow-rs/blob/725fe70db6efcd45bbdbdd6f3ef97c8c71221fad/parquet/src/arrow/arrow_writer/mod.rs#L3208-L3207
##########
parquet/src/arrow/arrow_writer/mod.rs:
##########
@@ -2496,26 +2472,14 @@ mod tests {
let string_view_values = StringViewArray::from(raw_string_values);
let binary_view_values =
BinaryViewArray::from_iter_values(raw_binary_values);
let nullable_string_view_values =
StringViewArray::from(nullable_string_values);
- let batch = RecordBatch::try_new(
- Arc::new(schema),
- vec![
- Arc::new(string_view_values),
- Arc::new(binary_view_values),
- Arc::new(nullable_string_view_values),
- ],
- )
- .unwrap();
- roundtrip(batch.clone(), Some(SMALL_SIZE / 2));
- roundtrip(batch, None);
+ RoundTripTest::new(Arc::new(string_view_values)).run();
Review Comment:
As above, this ctually increases coverage because it tries a both these two
sets of max row group sizes, and a bunch of other permutations
##########
parquet/src/arrow/arrow_writer/mod.rs:
##########
@@ -2261,15 +2257,11 @@ mod tests {
.build()
.unwrap();
let a = ListArray::from(a_list_data);
+ assert_eq!(a.null_count(), 1);
Review Comment:
as above, this actually increases coverage substantially
--
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]
