ARROW-10929: [Rust] Change CI to use Stable Rust#8930
ARROW-10929: [Rust] Change CI to use Stable Rust#8930alamb wants to merge 3 commits intoapache:masterfrom
Conversation
|
🤔 the windows CI build is failing when using stable: |
alamb
changed the title
alamb
force-pushed
the
ARROW-10929-stable-ci
branch
from
0493360 to
f3718ff
Compare
|
Hmm, I ran everything under valgrind locally and it did not detect any errors and the test has passed on windows a subsequent run. However, now I am getting a clippy error. I will fix that. |
I just saw this locally too and it passed on a second run. I filed https://issues.apache.org/jira/browse/ARROW-10943 |
alamb
force-pushed
the
ARROW-10929-stable-ci
branch
from
4df2551 to
5788152
Compare
| #![warn(missing_docs)] | ||
| // Clippy lints, some should be disabled incrementally | ||
| #![allow( | ||
| clippy::field_reassign_with_default, |
There was a problem hiding this comment.
this does not exist in stable clippy
|
|
||
| /// Represents some sort of execution plan, in String form | ||
| #[derive(Debug, Clone, PartialEq)] | ||
| #[allow(clippy::rc_buffer)] |
There was a problem hiding this comment.
Clippy is complaining about https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer - but I think in this case using the Arc is really about cheaply copying the String rather than it being mutable. The same happens for the other places I disabled this lint in this PR
I am not enough of a Rust expert to know if there is some better way to pass around a cheaply cloneable Vec or String that would appease Clippy.
There was a problem hiding this comment.
Did it give an alternative in the suggestion?
Looks like Arc<str> and Arc<[T]> should be the alternatives
There was a problem hiding this comment.
I agree @Dandandan . I changed then a couple of times, but since the focus of this PR was the CI in stable, which includes build, tests, etc. I considered clippy to be a minor hassle that we can deal with later.
There was a problem hiding this comment.
@Dandandan -- It did give alternatives but I did not figure out how to apply it.
The actually clippy lint didn't make a whole lot of sense to me as it seems to be focused on the mutability of the Arc/Rc's elements, which doesn't seem to apply here. From my reading of this code and the other callsites, the use case of using Arc<String> is not mutability of the contained String but rather cheaply copying it around.
Using Arc<str> doesn't make sense as the actual String is allocated at runtime (it is the formatted version of an explain plan)
There was a problem hiding this comment.
There is some discussion about why it can make sense to use Arc<str> over Arc<String>:
https://users.rust-lang.org/t/arc-string-vs-arc-str/30571
of course, difference is minimal, only saving a bit of indirection/some bytes.
You can use into to convert a String or &str into a Arc<str> or Rc<str>
https://doc.rust-lang.org/std/sync/struct.Arc.html#impl-From%3CString%3E
Codecov Report
@@ Coverage Diff @@
## master #8930 +/- ##
=======================================
Coverage 83.25% 83.25%
=======================================
Files 196 196
Lines 48116 48116
=======================================
Hits 40059 40059
Misses 8057 8057
Continue to review full report at Codecov.
|
jorgecarleitao
left a comment
jorgecarleitao
left a comment
There was a problem hiding this comment.
LGTM. Thanks for taking the time to address this. Good stuff!
6 participants
This is a cherry-pick of https://github.com/jorgecarleitao/arrow/commit/ca66d6d945e265dd2c83464bd80ff1dd7d231f7c by @jorgecarleitao
It runs all tests except the simd using
stable-- The SIMD feature still require nightly rust, but the default features do not (after #8698)Update: It also silences a few clippy lints which start complaining on stable -- I'll comment inline