refactor(cubesql): Fix clippy warnings in cubesql, part 1 (#9052)

* refactor(pg-srv): Fix redundant_slicing warning

* refactor(cubesql): Fix redundant_slicing warning

* refactor(pg-srv): Fix single_char_add_str warning

* refactor(pg-srv): Fix single_match warning

* refactor(pg-srv): Fix to_string_trait_impl warning

* refactor(cubesql): Fix assign_op_pattern warning

* refactor(cubesql): Fix bool_assert_comparison warning

* refactor(cubesql): Fix bool_comparison warning

* refactor(cubesql): Fix borrowed_box warning

* refactor(cubesql): Fix cast_abs_to_unsigned warning

* refactor(cubesql): Fix cmp_owned warning

* refactor(cubesql): Fix expect_fun_call warning

* refactor(cubesql): Fix explicit_auto_deref warning

* refactor(cubesql): Fix extra_unused_lifetimes warning

* refactor(cubesql): Fix filter_map_bool_then warning

* refactor(cubesql): Fix filter_map_identity warning

* refactor(cubesql): Simplify CloseCursor::All handling

* refactor(cubesql): Fix for_kv_map warning

* refactor(cubesql): Fix get_first warning

* refactor(cubesql): Fix identity_op warning

* refactor(cubesql): Enable manual_filter warning

* refactor(cubesql): Fix manual_is_ascii_check warning

* refactor(cubesql): Fix manual_map warning

* refactor(cubesql): Fix manual_strip warning

* refactor(cubesql): Fix to_string_in_format_args warning

Author: mcheshkov

rust/cubesql/cubesql/Cargo.toml

@@ -90,40 +90,22 @@ harness = false
 # Feel free to remove any rule from here and fix all warnings with it
 # Or to write a comment why rule should stay disabled
 [lints.clippy]
-assign_op_pattern = "allow"
-bool_assert_comparison = "allow"
-bool_comparison = "allow"
-borrowed_box = "allow"
-cast_abs_to_unsigned = "allow"
 clone_on_copy = "allow"
-cmp_owned = "allow"
 collapsible_if = "allow"
 collapsible_match = "allow"
 collapsible_else_if = "allow"
 comparison_chain = "allow"
 derive_ord_xor_partial_ord = "allow"
-expect_fun_call = "allow"
-explicit_auto_deref = "allow"
-extra_unused_lifetimes = "allow"
 field_reassign_with_default = "allow"
-filter_map_bool_then = "allow"
-filter_map_identity = "allow"
-for_kv_map = "allow"
-get_first = "allow"
-identity_op = "allow"
 if_same_then_else = "allow"
 into_iter_on_ref = "allow"
 iter_cloned_collect = "allow"
 iter_next_slice = "allow"
 len_without_is_empty = "allow"
 len_zero = "allow"
 let_and_return = "allow"
-manual_filter = "allow"
 manual_flatten = "allow"
-manual_is_ascii_check = "allow"
-manual_map = "allow"
 manual_range_contains = "allow"
-manual_strip = "allow"
 map_clone = "allow"
 map_flatten = "allow"
 map_identity = "allow"
@@ -152,11 +134,9 @@ redundant_closure = "allow"
 redundant_field_names = "allow"
 redundant_pattern = "allow"
 redundant_pattern_matching = "allow"
-redundant_slicing = "allow"
 result_large_err = "allow"
 single_match = "allow"
 should_implement_trait = "allow"
-to_string_in_format_args = "allow"
 to_string_trait_impl = "allow"
 too_many_arguments = "allow"
 type_complexity = "allow"

rust/cubesql/cubesql/e2e/tests/postgres.rs

@@ -83,7 +83,7 @@ impl PostgresIntegrationTestSuite {
             services.wait_processing_loops().await.unwrap();
         });
 
-        sleep(Duration::from_millis(1 * 1000)).await;
+        sleep(Duration::from_secs(1)).await;
 
         let client = PostgresIntegrationTestSuite::create_client(
             format!("host=127.0.0.1 port={} user=test password=test", port)
@@ -142,15 +142,15 @@ impl PostgresIntegrationTestSuite {
                         column.type_().oid(),
                         PgType::get_by_tid(
                             PgTypeId::from_oid(column.type_().oid())
-                                .expect(&format!("Unknown oid {}", column.type_().oid()))
+                                .unwrap_or_else(|| panic!("Unknown oid {}", column.type_().oid()))
                         )
                         .typname,
                     ));
                 }
 
                 // We dont need data when with_rows = false, but it's useful for testing that data type is correct
                 match PgTypeId::from_oid(column.type_().oid())
-                    .expect(&format!("Unknown type oid: {}", column.type_().oid()))
+                    .unwrap_or_else(|| panic!("Unknown type oid: {}", column.type_().oid()))
                 {
                     PgTypeId::INT8 => {
                         let value: Option<i64> = row.get(idx);
@@ -1269,7 +1269,7 @@ impl AsyncTestSuite for PostgresIntegrationTestSuite {
             |rows| {
                 assert_eq!(rows.len(), 1);
 
-                let columns = rows.get(0).unwrap().columns();
+                let columns = rows.first().unwrap().columns();
                 assert_eq!(
                     columns
                         .into_iter()

rust/cubesql/cubesql/src/compile/engine/df/scan.rs

@@ -751,11 +751,7 @@ impl CubeScanOneShotStream {
     }
 
     fn poll_next(&mut self) -> Option<ArrowResult<RecordBatch>> {
-        if let Some(batch) = self.data.take() {
-            Some(Ok(batch))
-        } else {
-            None
-        }
+        self.data.take().map(Ok)
     }
 }
 

rust/cubesql/cubesql/src/compile/engine/udf/common.rs

@@ -1418,7 +1418,7 @@ fn date_addsub_year_month(t: NaiveDateTime, i: i32, is_add: bool) -> Result<Naiv
     }
     debug_assert!(0 <= month);
     year += month / 12;
-    month = month % 12;
+    month %= 12;
 
     match change_ym(t, year, 1 + month as u32) {
         Some(t) => return Ok(t),
@@ -1442,13 +1442,13 @@ fn date_addsub_month_day_nano(
     let result = if month > 0 && is_add || month < 0 && !is_add {
         t.checked_add_months(Months::new(month as u32))
     } else {
-        t.checked_sub_months(Months::new(month.abs() as u32))
+        t.checked_sub_months(Months::new(month.unsigned_abs()))
     };
 
     let result = if day > 0 && is_add || day < 0 && !is_add {
         result.and_then(|t| t.checked_add_days(Days::new(day as u64)))
     } else {
-        result.and_then(|t| t.checked_sub_days(Days::new(day.abs() as u64)))
+        result.and_then(|t| t.checked_sub_days(Days::new(day.unsigned_abs() as u64)))
     };
 
     let result = result.and_then(|t| {
@@ -1472,7 +1472,7 @@ fn date_addsub_day_time(
     let result = if days > 0 && is_add || days < 0 && !is_add {
         t.checked_add_days(Days::new(days as u64))
     } else {
-        t.checked_sub_days(Days::new(days.abs() as u64))
+        t.checked_sub_days(Days::new(days.unsigned_abs() as u64))
     };
 
     let result = result.and_then(|t| {
@@ -1501,9 +1501,9 @@ fn last_day_of_month(y: i32, m: u32) -> u32 {
         return 31;
     }
     NaiveDate::from_ymd_opt(y, m + 1, 1)
-        .expect(&format!("Invalid year month: {}-{}", y, m))
+        .unwrap_or_else(|| panic!("Invalid year month: {}-{}", y, m))
         .pred_opt()
-        .expect(&format!("Invalid year month: {}-{}", y, m))
+        .unwrap_or_else(|| panic!("Invalid year month: {}-{}", y, m))
         .day()
 }
 
@@ -1564,10 +1564,7 @@ pub fn create_str_to_date_udf() -> ScalarUDF {
 
             let res = NaiveDateTime::parse_from_str(timestamp, &format).map_err(|e| {
                 DataFusionError::Execution(format!(
-                    "Error evaluating str_to_date('{}', '{}'): {}",
-                    timestamp,
-                    format,
-                    e.to_string()
+                    "Error evaluating str_to_date('{timestamp}', '{format}'): {e}"
                 ))
             })?;
 
@@ -1671,7 +1668,7 @@ pub fn create_to_char_udf() -> ScalarUDF {
                     let secs = duration.num_seconds();
                     let nanosecs = duration.num_nanoseconds().unwrap_or(0) - secs * 1_000_000_000;
                     let timestamp = NaiveDateTime::from_timestamp_opt(secs, nanosecs as u32)
-                        .expect(format!("Invalid secs {} nanosecs {}", secs, nanosecs).as_str());
+                        .unwrap_or_else(|| panic!("Invalid secs {} nanosecs {}", secs, nanosecs));
 
                     // chrono's strftime is missing quarter format, as such a workaround is required
                     let quarter = &format!("{}", timestamp.date().month0() / 3 + 1);
@@ -2237,10 +2234,7 @@ pub fn create_pg_get_constraintdef_udf() -> ScalarUDF {
         let oids_arr = downcast_primitive_arg!(args[0], "oid", OidType);
         let result = oids_arr
             .iter()
-            .map(|oid| match oid {
-                Some(_oid) => Some("PRIMARY KEY (oid)".to_string()),
-                _ => None,
-            })
+            .map(|oid| oid.map(|_oid| "PRIMARY KEY (oid)".to_string()))
             .collect::<StringArray>();
 
         Ok(Arc::new(result))
@@ -3583,7 +3577,7 @@ pub fn create_array_to_string_udf() -> ScalarUDF {
             let join_str = join_strs.value(i);
             let strings = downcast_string_arg!(array, "str", i32);
             let joined_string =
-                itertools::Itertools::intersperse(strings.iter().filter_map(|s| s), join_str)
+                itertools::Itertools::intersperse(strings.iter().flatten(), join_str)
                     .collect::<String>();
             builder.append_value(joined_string)?;
         }

rust/cubesql/cubesql/src/compile/engine/variable_provider.rs

@@ -25,7 +25,7 @@ impl VariablesProvider {
 
     fn get_session_value(&self, identifier: Vec<String>, var_type: VarType) -> Result<ScalarValue> {
         let key = if identifier.len() > 1 {
-            let ignore_first = identifier[0].to_ascii_lowercase() == "@@session".to_owned();
+            let ignore_first = identifier[0].to_ascii_lowercase() == "@@session";
             if ignore_first {
                 identifier[1..].concat()
             } else {
@@ -48,7 +48,7 @@ impl VariablesProvider {
 
     fn get_global_value(&self, identifier: Vec<String>) -> Result<ScalarValue> {
         let key = if identifier.len() > 1 {
-            let ignore_first = identifier[0].to_ascii_lowercase() == "@@global".to_owned();
+            let ignore_first = identifier[0].to_ascii_lowercase() == "@@global";
 
             if ignore_first {
                 identifier[1..].concat()
@@ -85,9 +85,7 @@ impl VarProvider for VariablesProvider {
 
         match (&first_word_vec[0], &first_word_vec[1]) {
             ('@', '@') => {
-                if identifier.len() > 1
-                    && identifier[0].to_ascii_lowercase() == "@@session".to_owned()
-                {
+                if identifier.len() > 1 && identifier[0].to_ascii_lowercase() == "@@session" {
                     return self.get_session_value(identifier, VarType::System);
                 }
 

rust/cubesql/cubesql/src/compile/parser.rs

@@ -23,16 +23,16 @@ impl Dialect for MySqlDialectWithBackTicks {
         // See https://dev.mysql.com/doc/refman/8.0/en/identifiers.html.
         // We don't yet support identifiers beginning with numbers, as that
         // makes it hard to distinguish numeric literals.
-        ('a'..='z').contains(&ch)
-            || ('A'..='Z').contains(&ch)
+        ch.is_ascii_lowercase()
+            || ch.is_ascii_uppercase()
             || ch == '_'
             || ch == '$'
             || ch == '@'
             || ('\u{0080}'..='\u{ffff}').contains(&ch)
     }
 
     fn is_identifier_part(&self, ch: char) -> bool {
-        self.is_identifier_start(ch) || ('0'..='9').contains(&ch)
+        self.is_identifier_start(ch) || ch.is_ascii_digit()
     }
 }
 
@@ -293,11 +293,9 @@ mod tests {
         );
         match result {
             Ok(_) => panic!("This test should throw an error"),
-            Err(err) => assert_eq!(
-                true,
-                err.to_string()
-                    .contains("Invalid query, no statements was specified")
-            ),
+            Err(err) => assert!(err
+                .to_string()
+                .contains("Invalid query, no statements was specified")),
         }
     }
 
@@ -310,11 +308,9 @@ mod tests {
         );
         match result {
             Ok(_) => panic!("This test should throw an error"),
-            Err(err) => assert_eq!(
-                true,
-                err.to_string()
-                    .contains("Multiple statements was specified in one query")
-            ),
+            Err(err) => assert!(err
+                .to_string()
+                .contains("Multiple statements was specified in one query")),
         }
     }
 
@@ -349,11 +345,9 @@ mod tests {
         );
         match result {
             Ok(_) => panic!("This test should throw an error"),
-            Err(err) => assert_eq!(
-                true,
-                err.to_string()
-                    .contains("Invalid query, no statements was specified")
-            ),
+            Err(err) => assert!(err
+                .to_string()
+                .contains("Invalid query, no statements was specified")),
         }
     }
 
@@ -366,11 +360,9 @@ mod tests {
         );
         match result {
             Ok(_) => panic!("This test should throw an error"),
-            Err(err) => assert_eq!(
-                true,
-                err.to_string()
-                    .contains("Multiple statements was specified in one query")
-            ),
+            Err(err) => assert!(err
+                .to_string()
+                .contains("Multiple statements was specified in one query")),
         }
     }
 

rust/cubesql/cubesql/src/compile/rewrite/analysis.rs

@@ -551,7 +551,7 @@ impl LogicalPlanAnalysis {
                         .unwrap();
                     let expr = original_expr(params[2])?;
                     map.push((
-                        Some(format!("{}.{}", cube, field_name.to_string())),
+                        Some(format!("{cube}.{field_name}")),
                         Member::VirtualField {
                             name: field_name.to_string(),
                             cube: cube.to_string(),

rust/cubesql/cubesql/src/compile/rewrite/rewriter.rs

@@ -550,10 +550,12 @@ impl egg::RewriteScheduler<LogicalPlanLanguage, LogicalPlanAnalysis> for Increme
         if iteration != self.current_iter {
             self.current_iter = iteration;
             self.current_eclasses.clear();
-            self.current_eclasses
-                .extend(egraph.classes().filter_map(|class| {
-                    (class.data.iteration_timestamp >= iteration).then(|| class.id)
-                }));
+            self.current_eclasses.extend(
+                egraph
+                    .classes()
+                    .filter(|class| (class.data.iteration_timestamp >= iteration))
+                    .map(|class| class.id),
+            );
         };
         assert_eq!(iteration, self.current_iter);
         rewrite.searcher.search_eclasses_with_limit(

rust/cubesql/cubesql/src/compile/rewrite/rules/filters.rs

@@ -2780,12 +2780,13 @@ impl FilterRules {
                                                     value.to_string()
                                                 }
                                             } else if op == "endsWith" || op == "notEndsWith" {
-                                                if value.starts_with("%") {
-                                                    let without_wildcard = value[1..].to_string();
+                                                if let Some(without_wildcard) =
+                                                    value.strip_prefix("%")
+                                                {
                                                     if without_wildcard.contains("%") {
                                                         continue;
                                                     }
-                                                    without_wildcard
+                                                    without_wildcard.to_string()
                                                 } else {
                                                     value.to_string()
                                                 }

rust/cubesql/cubesql/src/compile/router.rs

@@ -168,8 +168,7 @@ impl QueryRouter {
                 DatabaseProtocol::PostgreSQL,
             ) if object_type == &ast::ObjectType::Table => self.drop_table_to_plan(names).await,
             _ => Err(CompilationError::unsupported(format!(
-                "Unsupported query type: {}",
-                stmt.to_string()
+                "Unsupported query type: {stmt}"
             ))),
         };
 
@@ -348,7 +347,7 @@ impl QueryRouter {
             }
             DatabaseProtocol::MySQL => {
                 for key_value in key_values.iter() {
-                    if key_value.key.value.to_lowercase() == "autocommit".to_string() {
+                    if key_value.key.value.to_lowercase() == "autocommit" {
                         flags |= StatusFlags::AUTOCOMMIT;
 
                         break;
@@ -513,7 +512,7 @@ impl QueryRouter {
     async fn select_into_to_plan(
         &self,
         into: &ast::SelectInto,
-        query: &Box<ast::Query>,
+        query: &ast::Query,
         qtrace: &mut Option<Qtrace>,
         span_id: Option<Arc<SpanId>>,
     ) -> Result<QueryPlan, CompilationError> {
@@ -531,7 +530,7 @@ impl QueryRouter {
                 "query is unexpectedly not SELECT".to_string(),
             ));
         }
-        let new_stmt = ast::Statement::Query(new_query);
+        let new_stmt = ast::Statement::Query(Box::new(new_query));
         self.create_table_to_plan(&into.name, &new_stmt, qtrace, span_id)
             .await
     }