amadeus icon indicating copy to clipboard operation
amadeus copied to clipboard

Dynamic reads

Open robinbernon opened this issue 3 years ago • 3 comments

Adding ability to read a subset of parquet data dynamically.

robinbernon avatar Mar 25 '21 12:03 robinbernon

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?

alecmocatta avatar Apr 05 '21 11:04 alecmocatta

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?

robinbernon avatar Apr 08 '21 20:04 robinbernon

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).

alecmocatta avatar Apr 12 '21 10:04 alecmocatta