You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2026/08/07 14:32:55 UTC
[PR] Minor: migrate more arrow round trip tests to use RoundTripTest fixture [arrow-rs]
alamb opened a new pull request, #10585:
URL: https://github.com/apache/arrow-rs/pull/10585
# Which issue does this PR close?
- part of #10540
- Follow on to https://github.com/apache/arrow-rs/pull/10545
# Rationale for this change
There are 4000+ lines of tests in arrow_writer (and a bunch more in arrow_reader, I might point out) -- this makes it hard to find and add new tests or to evaluate test coverage
I encapsulated the testing harness into a struct in https://github.com/apache/arrow-rs/pull/10545 and now I want to (slowly) move more tests to use this new harness.
# What changes are included in this PR?
1. Migrate a few tests from using `roundtrip()` to using `RoundTripTest`; This actually increases coverage as I will descsribe below
# Are these changes tested?
yes by CI
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
If there are any breaking changes to public APIs, please call them out.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: migrate more arrow round trip tests to use RoundTripTest fixture [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: migrate more arrow round trip tests to use RoundTripTest fixture [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10585:
URL: https://github.com/apache/arrow-rs/pull/10585#issuecomment-5232736938
Thank you @Jefffrey
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: migrate more arrow round trip tests to use RoundTripTest fixture [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10585:
URL: https://github.com/apache/arrow-rs/pull/10585#issuecomment-5218526939
Well, since @Jefffrey approved this so fast I am now motivated to migrate some more. Be back shortly!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: migrate more arrow round trip tests to use RoundTripTest fixture [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #10585:
URL: https://github.com/apache/arrow-rs/pull/10585
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org