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

ARROW-10929: [Rust] Change CI to use Stable Rust #8930

Closed
wants to merge 3 commits into from

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Dec 15, 2020

This is a cherry-pick of jorgecarleitao@ca66d6d 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

@github-actions
Copy link

@alamb
Copy link
Contributor Author

alamb commented Dec 15, 2020

🤔 the windows CI build is failing when using stable:


failures:

---- encodings::encoding::tests::test_bool stdout ----
thread 'encodings::encoding::tests::test_bool' panicked at 'Invalid byte when reading bool', parquet\src\util\bit_util.rs:73:18

@alamb alamb changed the title ARROW-10929 [Rust] Change CI to use Stable Rust ARROW-10929: [Rust] Change CI to use Stable Rust Dec 15, 2020
@alamb
Copy link
Contributor Author

alamb commented Dec 16, 2020

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.

@andygrove
Copy link
Member

thinking the windows CI build is failing when using stable:


failures:

---- encodings::encoding::tests::test_bool stdout ----
thread 'encodings::encoding::tests::test_bool' panicked at 'Invalid byte when reading bool', parquet\src\util\bit_util.rs:73:18

I just saw this locally too and it passed on a second run. I filed https://issues.apache.org/jira/browse/ARROW-10943

@@ -17,7 +17,6 @@
#![warn(missing_docs)]
// Clippy lints, some should be disabled incrementally
#![allow(
clippy::field_reassign_with_default,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not exist in stable clippy

@@ -561,6 +561,7 @@ impl From<&PlanType> for String {

/// Represents some sort of execution plan, in String form
#[derive(Debug, Clone, PartialEq)]
#[allow(clippy::rc_buffer)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did it give an alternative in the suggestion?

Looks like Arc<str> and Arc<[T]> should be the alternatives

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Example:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=eb6db938ae03b001edca59aba1512d33

@codecov-io
Copy link

Codecov Report

Merging #8930 (5788152) into master (d65ba4e) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8930   +/-   ##
=======================================
  Coverage   83.25%   83.25%           
=======================================
  Files         196      196           
  Lines       48116    48116           
=======================================
  Hits        40059    40059           
  Misses       8057     8057           
Impacted Files Coverage Δ
rust/datafusion/src/logical_plan/expr.rs 76.76% <ø> (ø)
rust/datafusion/src/logical_plan/plan.rs 87.93% <ø> (ø)
rust/parquet/src/util/cursor.rs 83.18% <ø> (ø)
rust/parquet/src/util/memory.rs 89.57% <ø> (ø)
rust/arrow/src/array/array_struct.rs 88.68% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d65ba4e...5788152. Read the comment docs.

Copy link
Member

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for taking the time to address this. Good stuff!

@alamb alamb deleted the ARROW-10929-stable-ci branch December 21, 2020 12:22
GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 2021
This is a cherry-pick of jorgecarleitao@ca66d6d 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 apache#8698)

Update: It also silences a few clippy lints which start complaining on stable -- I'll comment inline

Closes apache#8930 from alamb/ARROW-10929-stable-ci

Lead-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Jorge C. Leitao <jorgecarleitao@gmail.com>
Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>
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.

6 participants