You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Phoenix500526 (via GitHub)" <gi...@apache.org> on 2026/09/23 02:37:10 UTC
[PR] test(parquet): move REE and basic roundtrips [arrow-rs]
Phoenix500526 opened a new pull request, #11184:
URL: https://github.com/apache/arrow-rs/pull/11184
# Which issue does this PR close?
Part of apache/arrow-rs#10540.
# Stack base
This draft PR targets `apache/arrow-rs:main`. Its review base is `consolidate-writer-roundtrip-II` (apache/arrow-rs#11183). Review the two commits added after that branch in the [stack delta](https://github.com/Phoenix500526/arrow-rs/compare/consolidate-writer-roundtrip-II...consolidate-writer-roundtrip-III).
# Rationale for this change
Continue moving writer roundtrip coverage in reviewable batches.
# What changes are included in this PR?
Move run-end-encoded (REE) and remaining basic writer roundtrip tests into `parquet/tests/arrow_writer/roundtrip.rs`. The two commits preserve their data comparisons and test options.
# Are these changes tested?
- `cargo test --locked --offline -p parquet --test arrow_writer` — 107 passed.
- `cargo test --locked --offline -p parquet --lib -- arrow::arrow_writer::tests` — 57 passed.
# Are there any user-facing changes?
No. This is a test-only move.
--
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] test(parquet): move REE and basic roundtrips —— Part III [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #11184:
URL: https://github.com/apache/arrow-rs/pull/11184#discussion_r4166078341
##########
parquet/src/arrow/arrow_writer/mod.rs:
##########
@@ -2077,12 +2077,14 @@ use crate as parquet_crate;
#[cfg(test)]
#[path = "../../../tests/arrow_writer/roundtrip_helpers.rs"]
+// The unit tests currently use only part of the shared helpers.
+#[expect(dead_code)]
Review Comment:
is this expected to be temporary or permanent? not too ideal that we need to place it on an entire module
--
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] test(parquet): move REE and basic roundtrips —— Part III [arrow-rs]
Posted by "Phoenix500526 (via GitHub)" <gi...@apache.org>.
Phoenix500526 commented on code in PR #11184:
URL: https://github.com/apache/arrow-rs/pull/11184#discussion_r4173331241
##########
parquet/tests/arrow_writer/roundtrip_helpers.rs:
##########
@@ -127,39 +109,12 @@ impl RoundTripTest {
}
}
- /// Set the schema
- pub(super) fn with_schema(mut self, schema: SchemaRef) -> Self {
- self.schema = Some(schema);
- self
- }
-
/// Set the nullable flag
pub(super) fn with_nullable(mut self, nullable: bool) -> Self {
self.nullable = nullable;
self
}
- /// Set bloom filter
- pub(super) fn with_bloom_filter(mut self, bloom_filter: bool) -> Self {
- self.bloom_filter = bloom_filter;
- self
- }
-
- /// Set bloom filter max ndv
- pub(super) fn with_bloom_filter_ndv(mut self, bloom_filter_ndv: u64) -> Self {
- self.bloom_filter_ndv = Some(bloom_filter_ndv);
Review Comment:
I tried putting #[expect(dead_code)] on the individual methods, but that moves the warning to the other test suite. For example, with_schema is unused in unit tests, so the expectation is satisfied there. In integration tests it is used, so Rust reports an unfulfilled_lint_expectations warning instead. The Bloom filter methods have the opposite problem.
Using #[allow(dead_code)] handles both cases, but conflicts with the project's clippy::allow_attributes lint and requires another exception.
Given that, I think the current split may be a better fit: keep the shared implementation centralized, and define the suite-specific methods alongside their callers. This avoids both module-wide suppression and additional lint exceptions.
--
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] test(parquet): move REE and basic roundtrips —— Part III [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #11184:
URL: https://github.com/apache/arrow-rs/pull/11184#discussion_r4190934969
##########
parquet/tests/arrow_writer/roundtrip_helpers.rs:
##########
@@ -127,39 +109,12 @@ impl RoundTripTest {
}
}
- /// Set the schema
- pub(super) fn with_schema(mut self, schema: SchemaRef) -> Self {
- self.schema = Some(schema);
- self
- }
-
/// Set the nullable flag
pub(super) fn with_nullable(mut self, nullable: bool) -> Self {
self.nullable = nullable;
self
}
- /// Set bloom filter
- pub(super) fn with_bloom_filter(mut self, bloom_filter: bool) -> Self {
- self.bloom_filter = bloom_filter;
- self
- }
-
- /// Set bloom filter max ndv
- pub(super) fn with_bloom_filter_ndv(mut self, bloom_filter_ndv: u64) -> Self {
- self.bloom_filter_ndv = Some(bloom_filter_ndv);
Review Comment:
i see, this is a bit tricky. in that case i think its preferable to still keep things centralized here in `roundtrip_helpers`, and for the specific methods annotate like so
```rust
// expect(dead_code) doesnt fire properly due to code being split across integration & unit tests
#[expect(clippy::allow_attributes)]
#[allow(dead_code)]
pub(super) fn with_schema(mut self, schema: SchemaRef) -> Self {
self.schema = Some(schema);
self
}
```
similar to how we do it here
https://github.com/apache/arrow-rs/blob/02accced63a992a20c91cbb5ac24261863531751/arrow-flight/tests/common/utils.rs#L28-L35
--
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] test(parquet): move REE and basic roundtrips —— Part III [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #11184:
URL: https://github.com/apache/arrow-rs/pull/11184#discussion_r4173179985
##########
parquet/tests/arrow_writer/roundtrip_helpers.rs:
##########
@@ -127,39 +109,12 @@ impl RoundTripTest {
}
}
- /// Set the schema
- pub(super) fn with_schema(mut self, schema: SchemaRef) -> Self {
- self.schema = Some(schema);
- self
- }
-
/// Set the nullable flag
pub(super) fn with_nullable(mut self, nullable: bool) -> Self {
self.nullable = nullable;
self
}
- /// Set bloom filter
- pub(super) fn with_bloom_filter(mut self, bloom_filter: bool) -> Self {
- self.bloom_filter = bloom_filter;
- self
- }
-
- /// Set bloom filter max ndv
- pub(super) fn with_bloom_filter_ndv(mut self, bloom_filter_ndv: u64) -> Self {
- self.bloom_filter_ndv = Some(bloom_filter_ndv);
Review Comment:
i think itd be good to keep them centrally defined here, but then put the `expect(dead_code)` on them directly if possible instead of just at the module level, so we at least have visibility on which methods it refers to
--
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] test(parquet): move REE and basic roundtrips —— Part III [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #11184:
URL: https://github.com/apache/arrow-rs/pull/11184#discussion_r4167672482
##########
parquet/tests/arrow_writer/roundtrip_helpers.rs:
##########
@@ -127,39 +109,12 @@ impl RoundTripTest {
}
}
- /// Set the schema
- pub(super) fn with_schema(mut self, schema: SchemaRef) -> Self {
- self.schema = Some(schema);
- self
- }
-
/// Set the nullable flag
pub(super) fn with_nullable(mut self, nullable: bool) -> Self {
self.nullable = nullable;
self
}
- /// Set bloom filter
- pub(super) fn with_bloom_filter(mut self, bloom_filter: bool) -> Self {
- self.bloom_filter = bloom_filter;
- self
- }
-
- /// Set bloom filter max ndv
- pub(super) fn with_bloom_filter_ndv(mut self, bloom_filter_ndv: u64) -> Self {
- self.bloom_filter_ndv = Some(bloom_filter_ndv);
Review Comment:
so we've split out these impls, because `with_schema` is only used in the integration tests whilst the rest are only used in the unit tests, right? is this split going to be permanent too, or a temporary measure as we move tests in follow up PRs?
--
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] test(parquet): move REE and basic roundtrips —— Part III [arrow-rs]
Posted by "Phoenix500526 (via GitHub)" <gi...@apache.org>.
Phoenix500526 commented on code in PR #11184:
URL: https://github.com/apache/arrow-rs/pull/11184#discussion_r4171144099
##########
parquet/tests/arrow_writer/roundtrip_helpers.rs:
##########
@@ -127,39 +109,12 @@ impl RoundTripTest {
}
}
- /// Set the schema
- pub(super) fn with_schema(mut self, schema: SchemaRef) -> Self {
- self.schema = Some(schema);
- self
- }
-
/// Set the nullable flag
pub(super) fn with_nullable(mut self, nullable: bool) -> Self {
self.nullable = nullable;
self
}
- /// Set bloom filter
- pub(super) fn with_bloom_filter(mut self, bloom_filter: bool) -> Self {
- self.bloom_filter = bloom_filter;
- self
- }
-
- /// Set bloom filter max ndv
- pub(super) fn with_bloom_filter_ndv(mut self, bloom_filter_ndv: u64) -> Self {
- self.bloom_filter_ndv = Some(bloom_filter_ndv);
Review Comment:
> so we've split out these impls, because `with_schema` is only used in the integration tests whilst the rest are only used in the unit tests, right?
Exactly.
> is this split going to be permanent too, or a temporary measure as we move tests in follow up PRs?
It will be permanent. Bloom filter-specific tests will stay in the unit test suite, while general data round-trip tests belong in the integration test suite. The two suites share the excution logic but define thir own purpose-specific configuration methods.
--
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] test(parquet): move REE and basic roundtrips —— Part III [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #11184:
URL: https://github.com/apache/arrow-rs/pull/11184#issuecomment-6009471604
thanks @Phoenix500526
--
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] test(parquet): move REE and basic roundtrips —— Part III [arrow-rs]
Posted by "Phoenix500526 (via GitHub)" <gi...@apache.org>.
Phoenix500526 commented on code in PR #11184:
URL: https://github.com/apache/arrow-rs/pull/11184#discussion_r4191305520
##########
parquet/tests/arrow_writer/roundtrip_helpers.rs:
##########
@@ -127,39 +109,12 @@ impl RoundTripTest {
}
}
- /// Set the schema
- pub(super) fn with_schema(mut self, schema: SchemaRef) -> Self {
- self.schema = Some(schema);
- self
- }
-
/// Set the nullable flag
pub(super) fn with_nullable(mut self, nullable: bool) -> Self {
self.nullable = nullable;
self
}
- /// Set bloom filter
- pub(super) fn with_bloom_filter(mut self, bloom_filter: bool) -> Self {
- self.bloom_filter = bloom_filter;
- self
- }
-
- /// Set bloom filter max ndv
- pub(super) fn with_bloom_filter_ndv(mut self, bloom_filter_ndv: u64) -> Self {
- self.bloom_filter_ndv = Some(bloom_filter_ndv);
Review Comment:
Updated
--
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] test(parquet): move REE and basic roundtrips —— Part III [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #11184:
URL: https://github.com/apache/arrow-rs/pull/11184
--
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] test(parquet): move REE and basic roundtrips —— Part III [arrow-rs]
Posted by "Phoenix500526 (via GitHub)" <gi...@apache.org>.
Phoenix500526 commented on code in PR #11184:
URL: https://github.com/apache/arrow-rs/pull/11184#discussion_r4166377369
##########
parquet/src/arrow/arrow_writer/mod.rs:
##########
@@ -2077,12 +2077,14 @@ use crate as parquet_crate;
#[cfg(test)]
#[path = "../../../tests/arrow_writer/roundtrip_helpers.rs"]
+// The unit tests currently use only part of the shared helpers.
+#[expect(dead_code)]
Review Comment:
Removed
--
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