Update rust vesion to 1.57 #1395

xudong963 · 2021-12-03T03:36:21Z

Which issue does this PR close?

Related: https://blog.rust-lang.org/2021/12/02/Rust-1.57.0.html

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

houqp · 2021-12-03T05:04:32Z

@xudong963 do you want to also fix the clippy errors?

xudong963 · 2021-12-03T05:07:33Z

@xudong963 do you want to also fix the clippy errors?

Sure 😄, a little late.

xudong963 · 2021-12-03T10:10:00Z

datafusion/src/scalar.rs

@@ -68,7 +68,7 @@ pub enum ScalarValue {
    /// large binary
    LargeBinary(Option<Vec<u8>>),
    /// list of nested ScalarValue (boxed to reduce size_of(ScalarValue))
-    #[allow(clippy::box_vec)]
+    #[allow(clippy::box_collection)]


https://rust-lang.github.io/rust-clippy/master/index.html#box_collection

Oh clippy, how wrong you are :)

the benefit is that the overall enum is smaller (as in the size of Vec<ScalarValue> is like 3 pointers (24 bytes) but a Box<Vec<ScalarValue>> is 8 bytes

xudong963 · 2021-12-03T10:10:36Z

datafusion/src/physical_plan/datetime_expressions.rs

@@ -77,7 +78,10 @@ where
        })?;

    // first map is the iterator, second is for the `Option<_>`
-    array.iter().map(|x| x.map(|x| op(x)).transpose()).collect()
+    array


https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure

xudong963 · 2021-12-03T10:11:22Z

datafusion/src/sql/parser.rs

@@ -85,7 +85,7 @@ pub struct CreateExternalTable {
 #[derive(Debug, Clone, PartialEq)]
 pub enum Statement {
    /// ANSI SQL AST node
-    Statement(SQLStatement),
+    Statement(Box<SQLStatement>),


https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant

xudong963 · 2021-12-03T10:41:15Z

I can't find field is never read clippy update in https://github.com/rust-lang/rust-clippy/blob/master/CHANGELOG.md#rust-157

So I still don't fix clippy errors in the screenshot.

alamb

Thank you @xudong963 -- there are still a few clippy failures https://github.com/apache/arrow-datafusion/runs/4406965470?check_suite_focus=true

They mostly look related to "fields not read" and some of that is surprising to me (like perhaps we have some missing functionality that needs to be fleshed out).

One approach we might take is add clippy allow lints and then go work through them one by one. I'll try and do this later today if I get to it

alamb · 2021-12-03T11:50:54Z

datafusion/src/scalar.rs

@@ -68,7 +68,7 @@ pub enum ScalarValue {
    /// large binary
    LargeBinary(Option<Vec<u8>>),
    /// list of nested ScalarValue (boxed to reduce size_of(ScalarValue))
-    #[allow(clippy::box_vec)]
+    #[allow(clippy::box_collection)]


Oh clippy, how wrong you are :)

the benefit is that the overall enum is smaller (as in the size of Vec<ScalarValue> is like 3 pointers (24 bytes) but a Box<Vec<ScalarValue>> is 8 bytes

xudong963 · 2021-12-03T12:39:53Z

One approach we might take is add clippy allow lints and then go work through them one by one. I'll try and do this later today if I get to it

You mean add #[allow(dead_code)]?

alamb · 2021-12-03T13:58:06Z

@xudong963 I pushed a few changes to this branch to try and get clippy to pass. Here's hoping we get a clean run

alamb · 2021-12-03T13:58:39Z

datafusion/src/physical_plan/expressions/average.rs

@@ -37,8 +37,8 @@ use super::{format_state_name, sum};
 #[derive(Debug)]
 pub struct Avg {
    name: String,
+    #[allow(dead_code)]


I will look into removing this field as well

This PR might be the reason causes the different behavior on dead_code.
rust-lang/rust#85200

I think you are right

alamb · 2021-12-03T13:58:52Z

datafusion/src/physical_plan/sort_preserving_merge.rs

@@ -346,7 +346,7 @@ struct SortPreservingMergeStream {
    receivers: Vec<mpsc::Receiver<ArrowResult<RecordBatch>>>,

    /// Drop helper for tasks feeding the [`receivers`](Self::receivers)
-    drop_helper: AbortOnDropMany<()>,
+    _drop_helper: AbortOnDropMany<()>,


this is used (its drop() impl is used) so change its name

alamb · 2021-12-03T13:59:31Z

ballista/rust/core/src/config.rs

@@ -31,22 +31,22 @@ pub const BALLISTA_DEFAULT_SHUFFLE_PARTITIONS: &str = "ballista.shuffle.partitio
 #[derive(Debug, Clone)]
 pub struct ConfigEntry {
    name: String,
-    description: String,
-    data_type: DataType,
+    _description: String,


I didn't know if this was important or not to keep in ballista so rather than removing it I renamed them to start with _ to satisfy clippy

Yes, very much agree with you, _ is a conservative but useful way!

alamb · 2021-12-03T14:00:22Z

datafusion/src/physical_plan/windows/built_in.rs

 }

 impl BuiltInWindowExpr {
    /// create a new built-in window function expression
    pub(super) fn new(
-        fun: BuiltInWindowFunction,


the fun and window_frame are used to construct the BuiltInWindowFunctionExpr (so the don't need to be also encoded on the BuildInWindowExpr

alamb · 2021-12-03T14:00:48Z

benchmarks/src/bin/tpch.rs

-    #[structopt(short = "s", long = "batch-size", default_value = "8192")]
-    batch_size: usize,
-
+    // /// Batch size when reading CSV or Parquet files


These parameters are never read (aka they don't do anything) so removing them from the CLI seemed like a reasonable thing to do

xudong963 · 2021-12-03T14:16:50Z

Nice, clippy passed😄

xudong963 · 2021-12-03T15:16:15Z

datafusion/src/physical_plan/expressions/average.rs

+        // Average is always Float64, but Avg::new() has a data_type
+        // parameter to keep a consistent signature with the other
+        // Aggregate expressions.
+        assert_eq!(data_type, DataType::Float64);


alamb · 2021-12-04T11:43:19Z

Thanks again @xudong963.

github-actions bot added ballista datafusion Changes in the datafusion crate labels Dec 3, 2021

xudong963 force-pushed the update_rust_version branch from 4e5fcbf to 9e893dc Compare December 3, 2021 10:00

github-actions bot added the sql SQL Planner label Dec 3, 2021

xudong963 force-pushed the update_rust_version branch from 9e893dc to bff569b Compare December 3, 2021 10:05

xudong963 commented Dec 3, 2021

View reviewed changes

Update rust vesion to 1.57

30dd9c9

xudong963 force-pushed the update_rust_version branch from bff569b to 30dd9c9 Compare December 3, 2021 10:34

alamb reviewed Dec 3, 2021

View reviewed changes

alamb mentioned this pull request Dec 3, 2021

Fix index out of bounds for stats on nested fields #1392

Closed

alamb added 3 commits December 3, 2021 08:50

Clean ups related to clippy

f17bb24

remove more unused fields

bfc372c

Remove some more unused code

31e6db8

alamb reviewed Dec 3, 2021

View reviewed changes

remove data_type from Average, but keep in consutrctor

4c8b337

alamb approved these changes Dec 3, 2021

View reviewed changes

xudong963 commented Dec 3, 2021

View reviewed changes

xudong963 mentioned this pull request Dec 3, 2021

Update rust version to 1.57 apache/arrow-rs#995

Closed

alamb mentioned this pull request Dec 3, 2021

support decimal scalar value #1394

Merged

houqp approved these changes Dec 4, 2021

View reviewed changes

alamb merged commit d047900 into apache:master Dec 4, 2021

xudong963 deleted the update_rust_version branch December 4, 2021 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update rust vesion to 1.57 #1395

Update rust vesion to 1.57 #1395

xudong963 commented Dec 3, 2021

houqp commented Dec 3, 2021

xudong963 commented Dec 3, 2021

xudong963 Dec 3, 2021

alamb Dec 3, 2021

xudong963 Dec 3, 2021

xudong963 Dec 3, 2021

xudong963 commented Dec 3, 2021

alamb left a comment

alamb Dec 3, 2021

xudong963 commented Dec 3, 2021

alamb commented Dec 3, 2021

alamb Dec 3, 2021

capkurmagati Dec 3, 2021

alamb Dec 3, 2021

alamb Dec 3, 2021

alamb Dec 3, 2021

xudong963 Dec 3, 2021

alamb Dec 3, 2021

alamb Dec 3, 2021

xudong963 commented Dec 3, 2021

xudong963 Dec 3, 2021

alamb commented Dec 4, 2021

Update rust vesion to 1.57 #1395

Update rust vesion to 1.57 #1395

Conversation

xudong963 commented Dec 3, 2021

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

houqp commented Dec 3, 2021

xudong963 commented Dec 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xudong963 commented Dec 3, 2021

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xudong963 commented Dec 3, 2021

alamb commented Dec 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xudong963 commented Dec 3, 2021

Choose a reason for hiding this comment

alamb commented Dec 4, 2021