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

Fix MSRV for skip-partial-aggregation #173

Closed
wants to merge 4 commits into from

Conversation

alamb
Copy link

@alamb alamb commented Aug 2, 2024

Which issue does this PR close?

This is a proposal to fix the MSRV failure on apache#11627

The failure is: https://github.com/apache/datafusion/actions/runs/10204347289/job/28280296775?pr=11627

Rationale for this change

CI is failing

To reproduce, use

andrewlamb@Andrews-MacBook-Pro-2:~/Software/datafusion$ cargo +1.76.0 check -p datafusion
warning: /Users/andrewlamb/Software/datafusion/Cargo.toml: unused manifest key: workspace.lints.rust.unexpected_cfgs.check-cfg
    Checking datafusion-common v40.0.0 (/Users/andrewlamb/Software/datafusion/datafusion/common)
    Checking datafusion-expr v40.0.0 (/Users/andrewlamb/Software/datafusion/datafusion/expr)
    Checking datafusion-execution v40.0.0 (/Users/andrewlamb/Software/datafusion/datafusion/execution)
    Checking datafusion-physical-expr-common v40.0.0 (/Users/andrewlamb/Software/datafusion/datafusion/physical-expr-common)
    Checking datafusion-sql v40.0.0 (/Users/andrewlamb/Software/datafusion/datafusion/sql)
    Checking datafusion-functions v40.0.0 (/Users/andrewlamb/Software/datafusion/datafusion/functions)
error[E0716]: temporary value dropped while borrowed
   --> datafusion/physical-expr-common/src/aggregate/groups_accumulator/prim_op.rs:167:18
    |
151 |         let values = match opt_filter {
    |             ------ borrow later stored here
...
167 |                 &PrimitiveArray::<T>::new(values_buf, nulls_buf).with_data_type(dt)
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
168 |             }
    |             - temporary value is freed at the end of this statement
    |
    = note: consider using a `let` binding to create a longer lived value

What changes are included in this PR?

Use a clone() (of a few Arc`s) to make the older rust compiler happy

Are these changes tested?

Are there any user-facing changes?

@korowa
Copy link
Owner

korowa commented Aug 3, 2024

Cherry-picked 191c3f5 with preserving an authorship into target branch, to avoid merging. Thank you!

@korowa korowa closed this Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants