jorgecarleitao / parquet2

Fastest and safest Rust implementation of parquet. `unsafe` free. Integration-tested against pyarrow
Other
356 stars 59 forks source link

Bump minimum async-stream version to 0.3.3 #212

Closed garrisonhess closed 1 year ago

garrisonhess commented 1 year ago

Hi @jorgecarleitao! I noticed a compiler error while upgrading arrow2 in a separate project that uses async-stream version 0.3.2. The other project's usage of version 0.3.2 led to arrow2 and parquet2 both using it, which then led to a compiler error. I looked into it and found that async-stream's 0.3.3 release contains a fix that parquet2 relies on. So this PR bumps parquet2's minimum async-stream version to 0.3.3 to include the fix.

Here is the code that fails to compile with async-stream 0.3.2. https://github.com/jorgecarleitao/parquet2/blob/ac31d0e8140c518509091c1ad17b9d716a728a70/src/read/page/stream.rs#L120-L126

Here is the resulting error:

error[E0277]: the `?` operator can only be used in an async block that returns `Result` or `Option` (or another type that implements `FromResidual`)
   --> /Users/gh/.cargo/registry/src/github.com-1ecc6299db9ec823/parquet2-0.17.1/src/read/page/stream.rs:126:14
    |
85  | /     try_stream! {
86  | |         while seen_values < total_num_values {
87  | |             // the header
88  | |             let page_header = read_page_header(reader, max_page_size).await?;
...   |
126 | |             )?;
    | |              ^ cannot use the `?` operator in an async block that returns `()`
127 | |         }
128 | |     }
    | |_____- this function should return `Result` or `Option` to accept `?`
    |
    = help: the trait `FromResidual<std::result::Result<Infallible, error::Error>>` is not implemented for `()`

This issue is fixed by https://github.com/tokio-rs/async-stream/pull/66, which is in the 0.3.3 release.

codecov-commenter commented 1 year ago

Codecov Report

Base: 85.05% // Head: 85.05% // No change to project coverage :thumbsup:

Coverage data is based on head (f01654f) compared to base (ac31d0e). Patch has no changes to coverable lines.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #212 +/- ## ======================================= Coverage 85.05% 85.05% ======================================= Files 86 86 Lines 8289 8289 ======================================= Hits 7050 7050 Misses 1239 1239 ``` Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jorgecarleitao commented 1 year ago

Hey! Thanks a lot for this. I think that #211 fixed this by not pinning us into 0.3.2. Could you confirm?

garrisonhess commented 1 year ago

Thanks for the quick reply!

Based on my reading of dependency version rules here, it seems to me that #211 would only reduce the minimum required version of async-stream to 0.3.0 from 0.3.2. Does that sound right? If that's correct, #211 wouldn't fix the issue because it wouldn't prevent parquet2 users from using 0.3.2.