Repository navigation
feat: allow downcasting ParquetFileReaderFactory - #26635
Conversation
github-actions
Bot
added
documentation
d36ed93 to
c9c6138
Compare
a0599b6 to
aa2ab01
Compare
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for adding concrete-type inspection to ParquetFileReaderFactory. The approach is sound and appropriately scoped. Extending the trait with Any provides a reliable way to identify built-in factories without changing reader execution or requiring existing implementations to change.
The architectural boundary is important for downstream scan rewrites: replacing a Parquet reader is safe only when the replacement can preserve the original reader's semantics. Concrete type identity provides a dependable basis for recognizing supported factories, unlike Debug formatting, which is neither stable nor authoritative. Keeping this capability on the owning trait also avoids introducing a parallel classification mechanism.
I found no blocking correctness issues. I've left a few non-blocking suggestions to strengthen regression coverage and place the upgrade note in the correct release guide.
| DefaultParquetFileReaderFactory::new(Arc::clone(&store)), | ||
| )); | ||
| let factory = source.parquet_file_reader_factory().unwrap(); | ||
| assert!(factory.is::<DefaultParquetFileReaderFactory>()); |
There was a problem hiding this comment.
Consider adding downcast_ref::<DefaultParquetFileReaderFactory>().is_some() here, and optionally the equivalent cached-factory assertion. The tests currently exercise only unsuccessful downcasts, so an implementation that always returns None would still pass.
| assert!( | ||
| factory | ||
| .downcast_ref::<DefaultParquetFileReaderFactory>() | ||
| .is_none() |
There was a problem hiding this comment.
As optional issue-specific coverage, consider testing a custom factory whose Debug output resembles a built-in factory. Assert that both built-in is::<T>() checks return false and that downcasting to the custom type succeeds, documenting the false-positive scenario from issue #26633 without requiring file I/O.
|
|
||
| See [issue #18235](https://github.com/apache/datafusion/issues/18235) for details. | ||
|
|
||
| ### `ParquetFileReaderFactory` adds `Any` as a supertrait |
There was a problem hiding this comment.
Could you move this entry to the 57.0.0 upgrade guide? The workspace is already at 57.0.0-dev (Cargo.toml:72), so documenting this API addition under 56.0.0 attributes it to the wrong release. The 57 guide does not yet exist in this checkout, so it would need to be created and registered using the existing upgrade-guide structure.
aa2ab01 to
d1873e9
Compare
Add an `Any` supertrait to `ParquetFileReaderFactory` and the `is` and `downcast_ref` methods that `dyn FileSource` has, so code that inspects a `ParquetSource` can tell `DefaultParquetFileReaderFactory` and `CachedParquetFileReaderFactory` from a caller-provided factory. The trait already required `'static`, which `Any` implies, so existing implementations compile unchanged; the explicit `'static` is dropped to match the other `Any` supertraits. The 57.0.0 upgrade guide notes the new supertrait. Co-Authored-By: Claude Opus 5.5 <[email protected]>
d1873e9 to
7f3d86e
Compare
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
@morristai, thanks for addressing the review feedback. The updated tests now cover successful downcasts for both built-in factories, cross-type rejection, and a custom factory with misleading Debug output. The upgrade note also targets the correct 57.0.0 release.
Using Any for concrete type checks is the right boundary. It lets plan rewrites identify supported reader factories without relying on Debug formatting. This matters because custom factories may change file contents, metadata, caching, or credentials, so they must not be treated as equivalent to built-in factories.
The follow-up addresses the previous concerns. The regression tests verify the type checks without requiring file I/O, and the documentation reflects the public API change. I found no remaining blocking issues. Approved.
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks good to me -- thank you @morristai and @kosiew
Which issue does this PR close?
ParquetFileReaderFactorylikeFileSourceandDataSource#26633.Rationale for this change
There is no way to tell which reader factory a
ParquetSourceuses.parquet_file_reader_factory()returns&Arc<dyn ParquetFileReaderFactory>, and the trait has no downcast path.What changes are included in this PR?
ParquetFileReaderFactorygetsAnyas a supertrait, anddyn ParquetFileReaderFactorygetsisanddowncast_ref, likedyn FileSourcein #21576. I also added a 57.0.0 upgrade guide entry, like the ones forMemoryPool(#21803) andQueryPlanner(#22241).What is the testing strategy for this PR?
A unit test in
source.rssets each built-in factory on aParquetSourceand checks thatparquet_file_reader_factory()downcasts to it.Are there any user-facing changes?
Two new methods on
dyn ParquetFileReaderFactory. The semver check will flag the new supertrait, but the trait already required'static, so existing implementations compile unchanged.Drafted with Claude. I reviewed the change and this text.
🤖 Generated with Claude Code