apache / arrow-rs

Official Rust implementation of Apache Arrow
https://arrow.apache.org/
Apache License 2.0
2.47k stars 735 forks source link

warning: methods `as_any` and `next_batch` are never used in `parquet` crate #6143

Open alamb opened 1 month ago

alamb commented 1 month ago

Describe the bug While publishing 52.2.0 I noticed this warning: #5998

I have also seen it locally

warning: methods `as_any` and `next_batch` are never used
  --> src/arrow/array_reader/mod.rs:64:8
   |
63 | pub trait ArrayReader: Send {
   |           ----------- methods in this trait
64 |     fn as_any(&self) -> &dyn Any;
   |        ^^^^^^
...
70 |     fn next_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {
   |        ^^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

warning: `parquet` (lib) generated 1 warning

To Reproduce

cargo test --test arrow_reader

Expected behavior No warnings

Additional context

etseidl commented 1 week ago

I've been playing around with this, and found that next_batch is only used in benchmarks and tests, while as_any truly does seem to be completely unused. For next_batch we can add #[cfg(any(feature = "experimental", test))] to silence the warning. For as_any, we could either remove it (it appears unused, and array_reader does not appear to be public), or simply allow dead code if we think as_any might be of use down the road. We could also just throw #[allow(dead_code)] on both. @alamb any preferences here?

alamb commented 1 week ago

Thanks @etseidl

I wonder if the issue is that the ArrayReader trait itself is not pub by default (maybe it needs an experimental flag?) - if it was a public trait then the error shouldn't happen

Maybe we can add a

#[cfg(any(feature = "experimental", test))]

To the overall trait?

etseidl commented 1 week ago

Maybe we can add a

#[cfg(any(feature = "experimental", test))]

To the overall trait?

Tried it and that caused a huge ripple (which I find odd since the whole module is marked experimental, but I think I still don't fully understand the mechanics there 🤷). I'm also puzzled over why the warnings starting appearing in the first place. Was it a code change or just improvements in unused code detection? I haven't wanted to start bisecting to run that thought to ground 😅.