Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 14 additions & 52 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ jobs:
strategy:
matrix:
arch: [amd64]
rust: [nightly-2020-11-24]
rust: [stable]
container:
image: ${{ matrix.arch }}/rust
steps:
Expand Down Expand Up @@ -77,7 +77,7 @@ jobs:
strategy:
matrix:
arch: [amd64]
rust: [nightly-2020-11-24]
rust: [stable]
container:
image: ${{ matrix.arch }}/rust
env:
Expand Down Expand Up @@ -114,11 +114,17 @@ jobs:
cd datafusion
cargo run --example csv_sql
cargo run --example parquet_sql
cd ..
cd arrow
cargo test
cargo run --example builders
cargo run --example dynamic_types
cargo run --example read_csv
cargo run --example read_csv_infer_schema

# test the --features "simd" of the arrow crate
# test the --features "simd" of the arrow crate. This requires nightly.
linux-test-simd:
name: AMD64 Debian 10 Rust ${{ matrix.rust }} test arrow simd
needs: [linux-build-lib]
runs-on: ubuntu-latest
strategy:
matrix:
Expand Down Expand Up @@ -156,57 +162,13 @@ jobs:
cd rust/arrow
cargo test --features "simd"

# test the arrow crate with stable rust
linux-test-stable:
name: AMD64 Debian 10 Rust ${{ matrix.rust }} test arrow
runs-on: ubuntu-latest
strategy:
matrix:
arch: [amd64]
rust: [stable]
container:
image: ${{ matrix.arch }}/rust
env:
ARROW_TEST_DATA: /__w/arrow/arrow/testing/data
steps:
- uses: actions/checkout@v2
with:
submodules: true
- name: Cache Cargo
uses: actions/cache@v2
with:
path: /github/home/.cargo
# this key equals the ones on `linux-build-lib` for re-use
key: cargo-cache-
- name: Cache Rust dependencies
uses: actions/cache@v2
with:
path: /github/home/target
# this key equals the ones on `linux-build-lib` for re-use
key: ${{ runner.os }}-${{ matrix.arch }}-target-cache-${{ matrix.rust }}
- uses: actions-rs/toolchain@v1
with:
toolchain: ${{ matrix.rust }}
default: true
components: rustfmt
- name: Run tests
run: |
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd rust/arrow
cargo test
cargo run --example builders
cargo run --example dynamic_types
cargo run --example read_csv
cargo run --example read_csv_infer_schema

windows-and-macos:
name: ${{ matrix.os }} Rust ${{ matrix.rust }}
runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [windows-latest, macos-latest]
rust: [nightly-2020-11-24]
rust: [stable]
steps:
- uses: actions/checkout@v2
with:
Expand Down Expand Up @@ -234,7 +196,7 @@ jobs:
strategy:
matrix:
arch: [amd64]
rust: [nightly-2020-11-24]
rust: [stable]
container:
image: ${{ matrix.arch }}/rust
steps:
Expand Down Expand Up @@ -271,7 +233,7 @@ jobs:
strategy:
matrix:
arch: [amd64]
rust: [nightly-2020-11-24]
rust: [stable]
steps:
- uses: actions/checkout@v2
with:
Expand Down Expand Up @@ -311,7 +273,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
rust: [nightly-2020-11-24]
rust: [stable]
steps:
- uses: actions/checkout@v2
with:
Expand Down
2 changes: 1 addition & 1 deletion rust/arrow/src/array/array_struct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ mod tests {
.null_count(2)
.null_bit_buffer(Buffer::from(&[9_u8]))
.add_buffer(Buffer::from(&[0, 3, 3, 3, 7].to_byte_slice()))
.add_buffer(Buffer::from("joemark".as_bytes()))
.add_buffer(Buffer::from(b"joemark"))
.build();

let expected_int_data = ArrayData::builder(DataType::Int32)
Expand Down
1 change: 0 additions & 1 deletion rust/datafusion/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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

clippy::float_cmp,
clippy::module_inception,
clippy::needless_lifetimes,
Expand Down
1 change: 1 addition & 0 deletions rust/datafusion/src/logical_plan/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -689,6 +689,7 @@ pub fn create_udf(

/// Creates a new UDAF with a specific signature, state type and return type.
/// The signature and state type must match the `Acumulator's implementation`.
#[allow(clippy::rc_buffer)]
pub fn create_udaf(
name: &str,
input_type: DataType,
Expand Down
1 change: 1 addition & 0 deletions rust/datafusion/src/logical_plan/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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

pub struct StringifiedPlan {
/// An identifier of what type of plan this string represents
pub plan_type: PlanType,
Expand Down
1 change: 1 addition & 0 deletions rust/parquet/src/util/cursor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use crate::file::writer::TryClone;
/// To achieve this, it uses Arc instead of shared references. Indeed reference fields are painfull
/// because the lack of Generic Associated Type implies that you would require complex lifetime propagation when
/// returning such a cursor.
#[allow(clippy::rc_buffer)]
pub struct SliceableCursor {
inner: Arc<Vec<u8>>,
start: u64,
Expand Down
1 change: 1 addition & 0 deletions rust/parquet/src/util/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ impl<T: Clone> Drop for Buffer<T> {
/// An representation of a slice on a reference-counting and read-only byte array.
/// Sub-slices can be further created from this. The byte array will be released
/// when all slices are dropped.
#[allow(clippy::rc_buffer)]
#[derive(Clone, Debug)]
pub struct BufferPtr<T> {
data: Arc<Vec<T>>,
Expand Down