Proposal: Rename FileSinkExec

[phillipleblanc]

Update (4/13/2024)

This was renamed to DataSinkExec in #10065

TL;DR:

I'm proposing to rename FileSinkExec to InsertExec or DataSinkExec (or another suitable name).

Why

In the middle of implementing TableProvider.insert_into for a TableProvider to write to a remote query engine, I searched the DataFusion codebase for examples of how this was done. I ran across FileSinkExec but dismissed it, since I wasn't writing to a file and I figured it was specific to that.

I ended up writing a custom Execution Plan that would perform the insert and return an Arrow record with the number of rows changed - but I felt that surely this was already done somewhere. I ended up finding the DataSink trait - which looked perfect for my use-case - and eventually realized that FileSinkExec was initially called InsertExec - which I would have used from the beginning if it had been called that. (Renamed in #7283)

As far as I can tell FileSinkExec isn't specific to files - but I'm still new to the DataFusion codebase, so I might be missing some context. I reworked my implementation to implement DataSink for the actual insert and use FileSinkExec in the TableProvider - and it all works.

Thoughts on renaming it to InsertExec or DataSinkExec?

[devinjdangelo]

FileSinkExec is used internally to DataFusion as the physical plan for inserting to ListingTables and writing out files via COPY statements. Both write out files, hence the name FileSinkExec.

I agree though that the plan can be used even more generally to sink to non file sources, so DataSinkExec could be a reasonable more general name.

[alamb]

I agree -- given FileSink doesn't actually depend on files (only on DataSink) calling it DataSinkExec makes sense to me: https://github.com/apache/arrow-datafusion/blob/2dad90425bacb98a3c2a4214faad53850c93104e/datafusion/physical-plan/src/insert.rs#L80-L91

If we are worried about backwards compatibility we could always leave a deprecated TypeDef in the code to help migration

LIke

#[deprecated("Use DataSinkExec instead")
type FileSinkExec = DataSinkExec;

[alamb]

Also potentially related is the discussion from @wiedld about supporting the file writer outside #9493