You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2025/02/03 15:10:37 UTC
[I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
alamb opened a new issue, #14444:
URL: https://github.com/apache/datafusion/issues/14444
### Is your feature request related to a problem or challenge?
Historically DataFusion was one (very) large crate `datafusion,` and as it grew bigger we extracted various functionality into separate crates. This leads to both faster compile times (as the crates can be compiled in parallel) as well easier to navigate code (as the crates force a cleaner dependency separation)
As described by @waynexia the build time of DataFusion has been growing,
- https://github.com/apache/datafusion/issues/14256
Some of this is due to the fact there is more code / more features to test. However a non trivial part of the long compile time is the time taken to compile the `datafusion` / core crate in https://github.com/apache/datafusion/tree/main/datafusion/core
<img width="2441" alt="Image" src="https://github.com/user-attachments/assets/fde0c93e-77d4-47fb-8daf-25c45c5513c1" />
While we are pursuing additional ways to reduce compile time, I think we should also move more code out of `datafusion/core` into their own crates.
We have successfully done this in the past with other projects such as
- https://github.com/apache/datafusion/issues/11502
- https://github.com/apache/datafusion/issues/10782
### Describe the solution you'd like
I would like to split out the https://github.com/apache/datafusion/tree/main/datafusion/core/src/datasource from DataFusion core
### Describe alternatives you've considered
I think we will end up with several new crates
- `datafusion-catalog-listing`: `ListingTable` and associated types like `PartitionedFile`
- `datafusion-datasource-parquet`: `ParquetExec` and file firmat
- `datafusion-datasource-avro` `AvroExec` and file formats
- `datafusion-datasource-arrow`
- `datafusion-datasource-json`
- `datafusion-datasource-csv`
I think we could start by creating `datafusion-catalog-listing` and trying to pull some of the listing table implementation into there and then trying to move one of the simpler datasources out (`datafusion-datasource-arrow` perhaps)
### Additional context
_No response_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2676870028
I'll try and take a stab at it, @alamb do you have a preference as to how many PRs I should break it into? There are no logical changes but I expect a very large numbers of small changes and would I love to do it in a way that you and others will be happy to review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2773359489
I think we have got most if not all low hanging fruits with #15459 .
I was thinking about how to make progress and here is what I though of so far:-
Originally, `get_file_format_factory` method needed to be moved to `Session` to remove downcast to `SessionState` at some places and move them out, but it has become apparent(unless I am missing something) that will not be possible without extreme measures.
So, I thought we could make another trait on `datasource` level to handle some of the `SessionState` stuff that inherits from Session.
That probably would not have been possible but since rust 1.86.0 rust supports trait_upcasting that means following syntax is now valid.
```rust
trait A { ... }
trait B: A {...}
...
let some_a_object: &dyn A = &some_b_object; // originally &dyn B now upcasted to &dyn A
```
here is PoC for that:-
https://github.com/logan-keede/datafusion/compare/move_memtable...logan-keede:datafusion:explore_session
I think we can not use this for sometime because of minimum rust compatibility commitments(AFAIK).
let me know If this makes sense, or if anybody has some other implementation/approach in mind.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2631281644
FYI @logan-keede this is the idea I was mentioning to you regarding more refactoring fun
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2692199949
The PR is out, but I suspect it's too big. I tried to get it in the 46 release, but now that I probably missed that, @alamb (or any other reviewers) would you prefer I split it up a bit?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2679630951
https://github.com/apache/datafusion/pull/14838 turned out to be easier than I thought, mostly because @logan-keede did much of the work to move test infrastructure over.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2688826808
Makes perfect sense, obviously smashing two crates together just to solve a dependency issue seems like an overreaction, especially as there are so many other dependencies that can be untangled first.
In the context of this work - I'll try and split the file formats tonight, they should be less tied to SessionState than ListingTable. After that's done we can circle back and try and focus more effort specifically on `SessionState`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2690829790
Yeah also checked and noticed that no other crate does that currently. I'll move the tests back to their original locations for now, some can be spared but most do seem to require `SessionState`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2631671091
take
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2690447654
I have a first version that seems to pass all tests/clippy, but I'm left with one issue before I publish it. In order to keep the tests in the format-specific crates, I pulled `core` as a dev-dependency using a `path`, so it can still be published.
Fixing `depcheck` to accept that is trivial, but I would love to know if that's an acceptable solution here or if I should move all tests up back to `core` where it has access to everything they need.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2783404784
There is still some non trivial code in the datasource crate here:
https://github.com/apache/datafusion/tree/main/datafusion/core/src/datasource
However, as you say breaking them out will require some fancier work / breaking up the APIs some more (I don't have time to really dig into this at the moment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed issue #14444: [Epic] Split datasources out from `datafusion` crate (`datafusion/core`)
URL: https://github.com/apache/datafusion/issues/14444
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-3180671853
Thanks again for all the help @logan-keede
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2687816143
> That's what I had in my mind too, I think the idea is to use Session trait, wherever SessionState is needed especially outside of core (and move those functions, if it is not already present).
Yes this is exactly right
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2676726335
I might not be able to contribute much till 7th March (due to institute commitments).
If anybody wants to continue on this issue, please feel free to do so. Here are the next steps that I had in my mind:-
1. Move remaining part of `FileScanConfig` and `FileStream` to `datasource` .
3. Move `FileSource` to `datasource`.
4. Move `FileFormat` to `datasource`.
5. Move `listing` and file format specific implementation to their own crates.
I would be happy to continue working on this as soon as I find some time, though that may only be after March 7.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2694410637
> The PR is out, but I suspect it's too big. I tried to get it in the 46 release, but now that I probably missed that, [@alamb](https://github.com/alamb) (or any other reviewers) would you prefer I split it up a bit?
I'll try and check it out today.
> Seems like that creates a dependency problem I'm not sure how to untangle. catalog::Session will need datasource::FiltFormatFactory, which needs FileFormat, which needs FileSource, which needs FileScanConfig.
I was thinking about this one.
It seems like `FileFormat` and `FileSource` may now have some non trivial overlap in functionalty. Maybe we need to rethink some of the APIs / abstractions 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "davisp (via GitHub)" <gi...@apache.org>.
davisp commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2638247527
Whoa, this is awesome!
I'm still ramping up on learning DataFusion internals as I add my own extensions and one thing that's been nagging at me is that it almost feels like the built-in data source providers are "cheating" because they get to live in core. Moving them all to separate crates that have to use the same interfaces as external datasources is something I've been contemplating suggesting for awhile so its great to see this already happening.
I'm definitely going to be keeping up and helping with this work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2639738923
> [@alamb](https://github.com/alamb) Can you point me at whatever tool you used to generate those compile timing graphs? Those look like something I'd absolutely adopt in a bunch of projects.
@davisp try this
```
rm -rf target
cargo build --release --timings --lib --quiet
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2640733024
> [@alamb](https://github.com/alamb) Can you point me at whatever tool you used to generate those compile timing graphs? Those look like something I'd absolutely adopt in a bunch of projects.
(and to be clear, that was @waynexia who made that chart for https://github.com/apache/datafusion/issues/14256-- I just copy pasted it here)
> I'm still ramping up on learning DataFusion internals as I add my own extensions and one thing that's been nagging at me is that it almost feels like the built-in data source providers are "cheating" because they get to live in core. Moving them all to separate crates that have to use the same interfaces as external datasources is something I've been contemplating suggesting for awhile so its great to see this already happening.
Awesome -- indeed this is the case
BTW there is a major refactor for datasources done by @mertak-synnada @ozankabak and others that (just) merged:
- https://github.com/apache/datafusion/pull/14224
Among other things it makes it easier to re-use common datasource features like pushdown, limit, etc
Now that that is in, we will be able to push this project along like a 🚀
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2676188953
> > [@logan-keede](https://github.com/logan-keede) as part of moving avro functionality into `datafusion-datasource-avro`, WDYT about putting all of it behind a feature flag, the same way `parquet` is now? Seems like its the only format/source that is visible with `default-features = false` but panics at runtime (for example in `AvroSource::create_file_opener`)
>
> I do not have much context on why it has been kept this way, but here is what _I_ think
>
> It seems like you can currently make a new `AvroSource` but not read an existing file. There might be some limited use case where user only needed to simulate `AvroSource` for some reason. (even that might crash as some other functionalities also panic). I think this use case is very limited and unintuitive and it is better to have it behind a feature flag(as you suggested), even if it is an API-Change.
>
> TL;DR: I think we can do that, but it seems like an API change so deprecation first.
>
> [@alamb](https://github.com/alamb) WDYT?
I think putting the entire AvroExec behind a feature flag makes sense even if it is an API change. I don't think we have to go through deprecation first as the change will be "add the flag"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-3180671329
Let's claim this epic is done for now and we can open a new one if/when someone wants to drive this forward more
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2676876878
> I'll try and take a stab at it, [@alamb](https://github.com/alamb) do you have a preference as to how many PRs I should break it into? There are no logical changes but I expect a very large numbers of small changes and would I love to do it in a way that you and others will be happy to review.
>
> edit: Started moving things around and seems like there a bunch of dependencies between them. I'll keep going until I get something that makes sense, hopefully the resulting PR won't be too big.
Thank you @AdamGS -- that is super helpful
In general, fewer smaller PRs is far easier to review. Also PRs that are mostly mechanical are also easy / fast to review
PRs that make changes that might have larger downstream implications are harder / take longer (as they require finding more focused time to review them)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "davisp (via GitHub)" <gi...@apache.org>.
davisp commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2640610691
@logan-keede Thanks for the tip!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2676185427
> [@logan-keede](https://github.com/logan-keede) as part of moving avro functionality into `datafusion-datasource-avro`, WDYT about putting all of it behind a feature flag, the same way `parquet` is now? Seems like its the only format/source that is visible with `default-features = false` but panics at runtime (for example in `AvroSource::create_file_opener`)
I do not have much context on why it has been kept this way, but here is what _I_ think
It seems like you can currently make a new `AvroSource` but not read an existing file. There might be some limited use case where user only needed to simulate `AvroSource` for some reason. (even that might crash as some other functionalities also panic).
I think this use case is very limited and unintuitive and it is better to have it behind a feature flag(as you suggested), even if it is an API-Change.
TL;DR: I think we can do that, but it seems like an API change so deprecation first.
@alamb WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2676158047
@logan-keede as part of moving avro functionality into `datafusion-datasource-avro`, WDYT about putting all of it behind a feature flag, the same way `parquet` is now? Seems like its the only format/source that is visible with `default-features = false` but panics at runtime (for example in `AvroSource::create_file_opener`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2688791206
>Seems like that creates a dependency problem I'm not sure how to untangle. catalog::Session will need datasource::FiltFormatFactory, which needs FileFormat, which needs FileSource, which needs FileScanConfig
I remember contemplating over that, I think I had a loose sketch but I don't recall anything directly useful at the moment.
> I can merge datafusion-datasource into datafusion-catalog, but also open to other ideas, and I'll also keep at it later today. Hopefully something will come up that won't require a huge breaking change.
1. I think we should try to avoid that mostly because it seems like a regression in term of modularity and code quality. (maybe we can only move some part of either or both crates up and down the dependency tree instead, not sure about this.)
2. It is my understanding that we do eventually want to move `SessionState` and everything else out of core, right?
Following on that,
I think the recent refactor has allowed us to move more stuff out of core (like `CteWorktable and `catalog-common`), which may also be direct or indirect dependency of `SessionState`.
I suggest that we take an Incremental approach and move those out first, that should help us identify what are the actual blockers* and then we can target them separately.
does that make sense?
*Highly coupled code that is hindering refactor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2654007044
Update here is that @logan-keede is cranking right along:
- https://github.com/apache/datafusion/pull/14616
After some discussion with @jayzhan211 I think we have a good plan going forward
I feel like we are on the cusp of finally getting these things split from the core 🙏
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2676927900
Ok I got a first draft that just moves `FileStream` and `FileScanConfig` and everything that comes with them. There are still some small issues to solve, mostly around test functionality that they pull from the core crate but expect that to be solvable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "davisp (via GitHub)" <gi...@apache.org>.
davisp commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2638265550
@alamb Can you point me at whatever tool you used to generate those compile timing graphs? Those look like something I'd absolutely adopt in a bunch of projects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2690796829
> I have a first version that seems to pass all tests/clippy, but I'm left with one issue before I publish it. In order to keep the tests in the format-specific crates, I pulled `core` as a dev-dependency using a `path`, so it can still be published. Fixing `depcheck` to accept that is trivial, but I would love to know if that's an acceptable solution here or if I should move all tests up back to `core` where it has access to everything they need.
referring to https://github.com/apache/datafusion/pull/14671#issuecomment-2660990592 and the fact that no crate has a dev-dependency on a crate above them in dependency tree (atleast none that I know of).
I think it is acceptable but not encouraged.
you may want to wait for a reply from @alamb or somebody else knowledgeable if you want to be sure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2688588184
Seems like that creates a dependency problem I'm not sure how to untangle. `catalog::Session` will need `datasource::FiltFormatFactory`, which needs `FileFormat`, which needs `FileSource`, which needs `FileScanConfig`.
I initially considered even returning `Any` instead of an `Ard<dyn FileFormatFactoy>` but many usages don't have a statically known format, and returning a `FileFormat` won't make a difference.
I can merge `datafusion-datasource` into `datafusion-catalog`, but also open to other ideas, and I'll also keep at it later today. Hopefully something will come up that won't require a huge breaking change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2654016041
This has been an update in plan, specifically addition of `datafusion-datasource` crate
- `datafusion-catalog-listing`: `ListingTable` and associated types like `PartitionedFile`
- `datafusion-datasource`: **NEW** that holds `FileCompressionType`, `PartitonedFile`,and `DataSource`
- `datafusion-datasource-parquet`: `ParquetExec` and file firmat
- `datafusion-datasource-avro` `AvroExec` and file formats
- `datafusion-datasource-arrow`
- `datafusion-datasource-json`
- `datafusion-datasource-csv`
_Originally posted by @alamb and suggested by @jayzhan211 in https://github.com/apache/datafusion/issues/14616#issuecomment-2653423772_
Please refer to the above mentioned issue for more context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2729189144
I was out for a couple of weeks on vacation and had some time to think, and what I came up with is to build this layer and maybe parts of IO around some abstracted columnar format, so more parts of these interfaces will be built on top of that.
Today the Parquet integration is way more complex than any of the other row-oriented formats, and when implementing [`Vortex`](https://github.com/spiraldb/vortex) support I find myself drawing inspiration from it, but much of that might as well be shared logic in datafusion.
I think @tustvold's [comment](https://github.com/apache/datafusion/pull/15018#issuecomment-2708822164) with regards to a new storage interface also touches it. We have opportunities to move the level of abstraction in a way that improves extendability AND make the system more performant at the same time. I've also been looking into caching of higher-level objects (like `ParquetMetaData`) and it seems likely that objects like will will exist for other columnar formats (schema + stats + some general file structure).
Maybe columnar and row-oriented formats can be split into different traits (maybe still sub-traits of `FileSource`), so we can abstract each one better (the way CSV and JSON already share the `Decoder` trait).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2682098263
I'll keep going later this week, my current plan is to split the rest of the work into three PRs:
1. `FileFormat`
2. `ListingTable` and friends.
3. Split out each individual format implementation into its own crate (csv, json, parquet, avro).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2687434700
Just updating that seems like steps 2 and 3 need to happen in one PR, otherwise a lot of code will move around multiple times which seems like unnecessary noise.
Another issue I've run into is that `ListingTable` (as well as csv, JSON and parquet) all downcast the session into a `SessionState` (see earlier discussions in #11600), which is currently core.
The only functionality they want is `SessionState::default_table_options` and `SessionState::default_table_options`, so the easiest thing to do will be to just move those functions to the trait. Given the amount of conversations/issues I've found about this point, I think its best to get a second opinion here before I start moving things around.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Split datasources out from `datafusion` crate (`datafusion/core`) [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on issue #14444:
URL: https://github.com/apache/datafusion/issues/14444#issuecomment-2687570685
> Another issue I've run into is that `ListingTable` (as well as csv, JSON and parquet) all downcast the session into a `SessionState` (see earlier discussions in [#11600](https://github.com/apache/datafusion/issues/11600) and [@logan-keede](https://github.com/logan-keede) 's [#14543](https://github.com/apache/datafusion/pull/14543)), which is currently core. The only functionality they want is `SessionState::default_table_options` and `SessionState::default_table_options`, so the easiest thing to do will be to just move those functions to the trait. Given the amount of conversations/issues I've found about this point, I think its best to get a second opinion here before I start moving things around.
That's what I had in my mind too, I think the idea is to use `Session` trait, wherever `SessionState` is needed especially outside of core (and move those functions, if it is not already present).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org