Add FileScanConfigBuilder
#15352
Add FileScanConfigBuilder
#15352
Conversation
github-actions
bot
added
core
| FileScanConfig::new(object_store_url, self.schema(), source) | ||
| FileScanConfigBuilder::new(object_store_url, self.schema(), source) | ||
| .with_projection(projection.cloned()) | ||
| .with_limit(limit); | ||
| .with_limit(limit) | ||
| .build(); |
There was a problem hiding this comment.
@mertak-synnada, hey, this PR is still WIP but I was wondering if you're happy with this approach. That's what we've discussed in #14685 (comment)
There was a problem hiding this comment.
Looking good so far, thank you! I haven't been able to test it thoroughly yet, but the legacy ParquetExecBuilder could be helpful for understanding specific cases, just FYI.
blaginin
changed the title
FileScanConfigBuilder and switch some casesFileScanConfigBuilder
|
also fyi @AdamGS 👀 |
|
|
||
| // Finally, put it all together into a DataSourceExec | ||
| Ok(file_scan_config.build()) | ||
| Ok(Arc::new(DataSourceExec::new(Arc::new(file_scan_config)))) |
There was a problem hiding this comment.
Isn't it possible to have this return as a function? Is it because of import cycles?
There was a problem hiding this comment.
you mean as aDataSourceExec function? It also looks a bit verbose to me, but the inner Arc is needed for dynamic dispatch, and the outer one makes the return type more explicit. Happy to make it DataSourceExec::new_arc if you want, but i don't think we use that a lot in datafusion
|
my 2c - this looks great, I would love to also rename |
Related to #14685 (comment)
Rationale for this change
FileScanConfignow violates single responsibility from SOLID. It serves two conflicting roles:As a builder, though this should be changed as discussed in
datafusion/datafusion/datasource/src/file_scan_config.rs
Line 631 in 635e73b
As a business logic provider (e.g.,
fn project,impl DataSource, etc.)These conflicting roles lead to issues like #14905 and #14679, where provider features are accessed even before the build process is complete.
What changes are included in this PR?
I've added
FileScanConfigBuilderand deprecated builder approach forFileScanConfigAre these changes tested?
Yes, updated exiting tests into the new interface
Are there any user-facing changes?
Yes, new builder interface - but the switch is quite easy (all builder-methods from
FileScanConfigare supported)