search

constellation-rs / amadeus

Harmonious distributed data analysis in Rust.
https://constellation.rs/amadeus
Apache License 2.0
472 stars 26 forks source link

Dynamic reads #132

Open robinbernon opened 3 years ago

robinbernon commented 3 years ago

Adding ability to read a subset of parquet data dynamically.

alecmocatta commented 3 years ago

The helpers here unfortunately break amadeus on stable. Ideally they impl serde_closure::traits::FnMut (which work on stable) rather than std::ops::FnMut (which don't yet). Implementing that is currently pretty rough https://github.com/constellation-rs/amadeus/blob/7f071aac9ae05a80c499ab3f1b24e9b69fc8ac6c/amadeus-serde/src/json.rs#L45-L51 I've been meaning to write another proc macro for serde_closure to make this easier but haven't had a chance yet.

The other alternative is to make helpers nightly-only, and hide it from the docs. What do you think?

robinbernon commented 3 years ago

The helpers here unfortunately break amadeus on stable. Ideally they impl serde_closure::traits::FnMut (which work on stable) rather than std::ops::FnMut (which don't yet). Implementing that is currently pretty rough

https://github.com/constellation-rs/amadeus/blob/7f071aac9ae05a80c499ab3f1b24e9b69fc8ac6c/amadeus-serde/src/json.rs#L45-L51

I've been meaning to write another proc macro for serde_closure to make this easier but haven't had a chance yet. The other alternative is to make helpers nightly-only, and hide it from the docs. What do you think?

Issue with impl serde closure instead of normal closure is that it would mean all adapter operations for parallel streams would have to change to using serde closures to support the change. There is currently a noticeable lack of intellisense when working with macros even on the best available IDE's - due to this I'd personally prefer to keep the standard closures in parallel streams as I like having intellisense when working on the various adapter operations etc before moving them into the required serde macros for distributed processing. Any chaance I can include the helpers here as a nightly feature then?

alecmocatta commented 3 years ago

it would mean all adapter operations for parallel streams would have to change to using serde closures to support the change

Oh right, I see the issue. Yes agreed it's strongly desirable to keep std::ops::Fn* on parallel streams. Given this, if we do merge these helpers they will need to be behind a cfg(nightly).