Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stack overflows and large_futures clippy errors on v37 #9893

Closed
Tracked by #9904
sergiimk opened this issue Mar 31, 2024 · 3 comments · Fixed by #10033
Closed
Tracked by #9904

Stack overflows and large_futures clippy errors on v37 #9893

sergiimk opened this issue Mar 31, 2024 · 3 comments · Fixed by #10033
Labels
bug Something isn't working

Comments

@sergiimk
Copy link
Contributor

sergiimk commented Mar 31, 2024

Describe the bug

Creating a quick issue to point out a possible problem in 37.0.0-rc1.

After upgrading to this tag we saw a huge number of large_futures errors from clippy.

When we bumped the threshold to ~30kb we started seeing stack overflows in our tests.

To Reproduce

In our case the problem seems to stem from DataFrame::write_parquet method - it is the first one to be flagged by clippy.

I am still investigating the cause and will update the ticket if I find something.

Expected behavior

No response

Additional context

No response

@sergiimk sergiimk added the bug Something isn't working label Mar 31, 2024
@sergiimk
Copy link
Contributor Author

sergiimk commented Mar 31, 2024

Related issues:

So the problem is already known, and RUST_MIN_STACK was already increased in CI.

Although I didn't find an explanation why the situation got significantly worse between v36 and v37. On v36 we haven't even been seeing any clippy warnings, so we went from nothing to stack overflows in one release.

Stack overflows not in some heavy benchmarks but on tests that transform just 5-10 records.

@sergiimk
Copy link
Contributor Author

Even a simple function like this:

pub async fn assert_dfs_equal(lhs: DataFrame, rhs: DataFrame) {
    pretty_assertions::assert_eq!(lhs.schema(), rhs.schema());

    let lhs_batches = lhs.collect().await.unwrap();
    let rhs_batches = rhs.collect().await.unwrap();

    let lhs_str = datafusion::arrow::util::pretty::pretty_format_batches(&lhs_batches)
        .unwrap()
        .to_string();

    let rhs_str = datafusion::arrow::util::pretty::pretty_format_batches(&rhs_batches)
        .unwrap()
        .to_string();

    pretty_assertions::assert_eq!(lhs_str, rhs_str);
}

Gives:

warning: large future with a size of 17360 bytes
   --> src/infra/ingest-datafusion/tests/utils/mod.rs:195:5
    |
195 |     assert_dfs_equal(a, b).await;
    |     ^^^^^^^^^^^^^^^^^^^^^^ help: consider `Box::pin` on it: `Box::pin(assert_dfs_equal(a, b))`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_futures

After changing to Box::pin(df.collect()).await warning goes away, so likely some large state is being stored on stack during plan prep/execution.

@sergiimk sergiimk changed the title Stack overflows and large_futures clippy errors on v37-rc1 Stack overflows and large_futures clippy errors on v37 Apr 7, 2024
@sergiimk
Copy link
Contributor Author

sergiimk commented Apr 7, 2024

Confirmed that the issue is present on v37 release.

Getting thread 'x' has overflowed its stack consistently in tests during the query optimization phase.

For us tweaking the stack size is not a viable option. Will be skipping the upgrade and try to find time to investigate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant