Clean up APIs around `FileScanConfigBuilder`, `FileScanConfig` and `FileSource`

[adriangb]

In particular, passing around of schema is a bit wonky, see https://github.com/apache/datafusion/pull/15769/files#r2052245527.

If we force passing file_schema into ParquetFileSource::new and such that means as a user you have to pass it into two places.

I think the real solution is to have a FileSourceBuilder trait that has build() method that accepts file_schema and produces a FileScan. Then FileScanConfigBuilder can call that? It might be too much API churn to be worth it.

[adriangb]

My proposal for this which I've put in some other places:

1. Make good shared helpers for the things that the data sources do in common.
2. We have a good story for filter pushdown already I think.
3. For projection pushdown I think we should make some of the stuff in ProjectionExec public / easy to share.
4. Touch up FileScanConfig as a shared builder / config for all sources (excluding things like CSV or parquet specific options)
5. Move logic from DataSourceExec / FileScanConfig into the individual FileSources
6. Rip out FileScanConfig from the execution pathway
7. Do little refactors like Pass batch_size directly when creating file opener #17076

One question I'm not sure about is how to go about all of this: small changes, trying to keep backwards compatibility but necessarily breaking a lot of things, or make a new parallel implementation where we try to preserve the high level APIs and then eventually deprecate / replace the current ones. It seems to be what we did last refactor and based on discussion in #17076 may be the easiest path forward.

[berkaysynnada]

@adriangb thank you for sharing it. I don’t have much time nowadays to iterate on the new design proposal, but once it’s in a ready state, can you please ping me for a review? These api's are very critical for us (and I’m sure for many others)

[adriangb]

@adriangb thank you for sharing it. I don’t have much time nowadays to iterate on the new design proposal, but once it’s in a ready state, can you please ping me for a review? These api's are very critical for us (and I’m sure for many others)

Yes will do

[friendlymatthew]

Hi, I've been staring at FileScanConfig for the past day now and had some thoughts about the redesign.

One approach (bad): create a new struct FileScan over the config

One thing I tried was adding a wrapper around FileScanConfig called FileScan. FileScan would impl DataSource and look something like:

struct FileScan {
    file_source: Arc<dyn FileSource>,
    config: FileScanConfig
}

This way, rather than passing the parent struct (FileScanConfig) into the child struct (FileSource) when doing file operations, we'd pass a sibling struct. And technically by extracting file_source out of FileScanConfig, the data execution flow shouldn't touch the config.

However, I'm dubious whether this refactor will do anything meaningful. We're introducing another struct to be explicit about what implements DataSource, but we haven't really done anything major to FileScanConfig.

Another approach (better): let ParquetSource, JsonSource, etc directly impl DataSource

From what I understand, prior versions had file-specific execution plans (ParquetExec, JsonExec, etc..) and the existing version was written to bridge similar functionality.

However, it seems like each file source has pretty different characteristics/implementations. Now, we have to bear this abstraction cost by maintaining a format-agnostic config (FileScanConfig) with format-specific fields like new_lines_in_values or expr_adapter_factory that is only specific to 1 file source. We also introduce various call sites throughout the FileScanConfig -> FileSource relationship that does the same thing like setting the batch size/statistics/projection.

I propose we directly implement DataSource for all structs that impl FileSource. Each format already has most of what they need to implement DataSource already! This way, we'll at least narrow down which fields are shared throughout all file sources. Note, the refactor will still allow us to do things like #17095

cc @adriangb @comphead @blaginin @xudong963 @mbutrovich