Use partial_cmp and make -i case insensitive (#4498)

* Use partial_cmp and make -i case insensitive

* Insensitive sort multiple columns

crates/nu-command/src/filters/sort_by.rs

@@ -1,10 +1,9 @@
-use chrono::{DateTime, FixedOffset};
 use nu_engine::{column::column_does_not_exist, CallExt};
 use nu_protocol::{
     ast::Call,
     engine::{Command, EngineState, Stack},
-    Category, Config, Example, IntoInterruptiblePipelineData, PipelineData, ShellError, Signature,
-    Span, SyntaxShape, Value,
+    Category, Example, IntoInterruptiblePipelineData, PipelineData, ShellError, Signature, Span,
+    SyntaxShape, Value,
 };
 use std::cmp::Ordering;
 
@@ -112,10 +111,9 @@ impl Command for SortBy {
         let reverse = call.has_flag("reverse");
         let insensitive = call.has_flag("insensitive");
         let metadata = &input.metadata();
-        let config = stack.get_config()?;
         let mut vec: Vec<_> = input.into_iter().collect();
 
-        sort(&mut vec, columns, call, insensitive, &config)?;
+        sort(&mut vec, columns, call.head, insensitive)?;
 
         if reverse {
             vec.reverse()
@@ -134,9 +132,8 @@ impl Command for SortBy {
 pub fn sort(
     vec: &mut [Value],
     columns: Vec<String>,
-    call: &Call,
+    span: Span,
     insensitive: bool,
-    config: &Config,
 ) -> Result<(), ShellError> {
     if vec.is_empty() {
         return Err(ShellError::LabeledError(
@@ -153,11 +150,11 @@ pub fn sort(
         } => {
             if columns.is_empty() {
                 println!("sort-by requires a column name to sort table data");
-                return Err(ShellError::CantFindColumn(call.head, call.head));
+                return Err(ShellError::CantFindColumn(span, span));
             }
 
             if column_does_not_exist(columns.clone(), cols.to_vec()) {
-                return Err(ShellError::CantFindColumn(call.head, call.head));
+                return Err(ShellError::CantFindColumn(span, span));
             }
 
             // check to make sure each value in each column in the record
@@ -180,33 +177,32 @@ pub fn sort(
                     .iter()
                     .all(|x| matches!(x.get_type(), nu_protocol::Type::String));
 
-            vec.sort_by(|a, b| {
-                process(
-                    a,
-                    b,
-                    &columns[0],
-                    call,
-                    should_sort_case_insensitively,
-                    config,
-                )
-                .expect("sort_by Value::Record bug")
-            });
+            vec.sort_by(|a, b| process(a, b, &columns, span, should_sort_case_insensitively));
         }
         _ => {
             vec.sort_by(|a, b| {
                 if insensitive {
-                    let lowercase_left = Value::string(
-                        a.into_string("", config).to_ascii_lowercase(),
-                        Span::test_data(),
-                    );
-                    let lowercase_right = Value::string(
-                        b.into_string("", config).to_ascii_lowercase(),
-                        Span::test_data(),
-                    );
-                    coerce_compare(&lowercase_left, &lowercase_right, call)
-                        .expect("sort_by default bug")
+                    let lowercase_left = match a {
+                        Value::String { val, span } => Value::String {
+                            val: val.to_ascii_lowercase(),
+                            span: *span,
+                        },
+                        _ => a.clone(),
+                    };
+
+                    let lowercase_right = match b {
+                        Value::String { val, span } => Value::String {
+                            val: val.to_ascii_lowercase(),
+                            span: *span,
+                        },
+                        _ => b.clone(),
+                    };
+
+                    lowercase_left
+                        .partial_cmp(&lowercase_right)
+                        .unwrap_or(Ordering::Equal)
                 } else {
-                    coerce_compare(a, b, call).expect("sort_by default bug")
+                    a.partial_cmp(b).unwrap_or(Ordering::Equal)
                 }
             });
         }
@@ -217,150 +213,53 @@ pub fn sort(
 pub fn process(
     left: &Value,
     right: &Value,
-    column: &str,
-    call: &Call,
+    columns: &[String],
+    span: Span,
     insensitive: bool,
-    config: &Config,
-) -> Result<Ordering, ShellError> {
-    let left_value = left.get_data_by_key(column);
+) -> Ordering {
+    for column in columns {
+        let left_value = left.get_data_by_key(column);
 
-    let left_res = match left_value {
-        Some(left_res) => left_res,
-        None => Value::Nothing { span: call.head },
-    };
+        let left_res = match left_value {
+            Some(left_res) => left_res,
+            None => Value::Nothing { span },
+        };
 
-    let right_value = right.get_data_by_key(column);
+        let right_value = right.get_data_by_key(column);
 
-    let right_res = match right_value {
-        Some(right_res) => right_res,
-        None => Value::Nothing { span: call.head },
-    };
+        let right_res = match right_value {
+            Some(right_res) => right_res,
+            None => Value::Nothing { span },
+        };
 
-    if insensitive {
-        let lowercase_left = Value::string(
-            left_res.into_string("", config).to_ascii_lowercase(),
-            Span::test_data(),
-        );
-        let lowercase_right = Value::string(
-            right_res.into_string("", config).to_ascii_lowercase(),
-            Span::test_data(),
-        );
-        coerce_compare(&lowercase_left, &lowercase_right, call)
-    } else {
-        coerce_compare(&left_res, &right_res, call)
-    }
-}
+        let result = if insensitive {
+            let lowercase_left = match left_res {
+                Value::String { val, span } => Value::String {
+                    val: val.to_ascii_lowercase(),
+                    span,
+                },
+                _ => left_res,
+            };
 
-#[derive(Debug)]
-pub enum CompareValues {
-    Ints(i64, i64),
-    Floats(f64, f64),
-    String(String, String),
-    Booleans(bool, bool),
-    Filesize(i64, i64),
-    Date(DateTime<FixedOffset>, DateTime<FixedOffset>),
-}
-
-impl CompareValues {
-    pub fn compare(&self) -> std::cmp::Ordering {
-        match self {
-            CompareValues::Ints(left, right) => left.cmp(right),
-            CompareValues::Floats(left, right) => process_floats(left, right),
-            CompareValues::String(left, right) => left.cmp(right),
-            CompareValues::Booleans(left, right) => left.cmp(right),
-            CompareValues::Filesize(left, right) => left.cmp(right),
-            CompareValues::Date(left, right) => left.cmp(right),
+            let lowercase_right = match right_res {
+                Value::String { val, span } => Value::String {
+                    val: val.to_ascii_lowercase(),
+                    span,
+                },
+                _ => right_res,
+            };
+            lowercase_left
+                .partial_cmp(&lowercase_right)
+                .unwrap_or(Ordering::Equal)
+        } else {
+            left_res.partial_cmp(&right_res).unwrap_or(Ordering::Equal)
+        };
+        if result != Ordering::Equal {
+            return result;
         }
     }
-}
 
-pub fn process_floats(left: &f64, right: &f64) -> std::cmp::Ordering {
-    let result = left.partial_cmp(right);
-    match result {
-        Some(Ordering::Greater) => Ordering::Greater,
-        Some(Ordering::Less) => Ordering::Less,
-        _ => Ordering::Equal,
-    }
-}
-
-/*
-Arbitrary Order of Values:
-    Floats
-    Ints
-    Strings
-    Bools
-    Lists
-*/
-
-pub fn coerce_compare(left: &Value, right: &Value, call: &Call) -> Result<Ordering, ShellError> {
-    Ok(match (left, right) {
-        (Value::Float { val: left, .. }, Value::Float { val: right, .. }) => {
-            CompareValues::Floats(*left, *right).compare()
-        }
-        (Value::Filesize { val: left, .. }, Value::Filesize { val: right, .. }) => {
-            CompareValues::Filesize(*left, *right).compare()
-        }
-        (Value::Date { val: left, .. }, Value::Date { val: right, .. }) => {
-            CompareValues::Date(*left, *right).compare()
-        }
-        (Value::Int { val: left, .. }, Value::Int { val: right, .. }) => {
-            CompareValues::Ints(*left, *right).compare()
-        }
-        (Value::String { val: left, .. }, Value::String { val: right, .. }) => {
-            CompareValues::String(left.clone(), right.clone()).compare()
-        }
-        (Value::Bool { val: left, .. }, Value::Bool { val: right, .. }) => {
-            CompareValues::Booleans(*left, *right).compare()
-        }
-
-        // FIXME: Not sure how to compare and sort lists
-        (Value::List { .. }, Value::List { .. }) => Ordering::Equal,
-
-        // Floats will always come before Ints
-        (Value::Float { .. }, Value::Int { .. }) => Ordering::Less,
-        (Value::Int { .. }, Value::Float { .. }) => Ordering::Greater,
-
-        // Floats will always come before Strings
-        (Value::Float { .. }, Value::String { .. }) => Ordering::Less,
-        (Value::String { .. }, Value::Float { .. }) => Ordering::Greater,
-
-        // Floats will always come before Bools
-        (Value::Float { .. }, Value::Bool { .. }) => Ordering::Less,
-        (Value::Bool { .. }, Value::Float { .. }) => Ordering::Greater,
-
-        // Floats will always come before Lists
-        (Value::Float { .. }, Value::List { .. }) => Ordering::Less,
-        (Value::List { .. }, Value::Float { .. }) => Ordering::Greater,
-
-        // Ints will always come before strings
-        (Value::Int { .. }, Value::String { .. }) => Ordering::Less,
-        (Value::String { .. }, Value::Int { .. }) => Ordering::Greater,
-
-        // Ints will always come before Bools
-        (Value::Int { .. }, Value::Bool { .. }) => Ordering::Less,
-        (Value::Bool { .. }, Value::Int { .. }) => Ordering::Greater,
-
-        // Ints will always come before Lists
-        (Value::Int { .. }, Value::List { .. }) => Ordering::Less,
-        (Value::List { .. }, Value::Int { .. }) => Ordering::Greater,
-
-        // Strings will always come before Bools
-        (Value::String { .. }, Value::Bool { .. }) => Ordering::Less,
-        (Value::Bool { .. }, Value::String { .. }) => Ordering::Greater,
-
-        // Strings will always come before Lists
-        (Value::String { .. }, Value::List { .. }) => Ordering::Less,
-        (Value::List { .. }, Value::String { .. }) => Ordering::Greater,
-
-        // Bools will always come before Lists
-        (Value::Bool { .. }, Value::List { .. }) => Ordering::Less,
-        (Value::List { .. }, Value::Bool { .. }) => Ordering::Greater,
-
-        _ => {
-            let description = format!("not able to compare {:?} with {:?}\n", left, right);
-            return Err(ShellError::TypeMismatch(description, call.head));
-        }
-    })
+    Ordering::Equal
 }
 
 #[cfg(test)]

crates/nu-command/tests/commands/sort_by.rs

@@ -127,7 +127,7 @@ fn ls_sort_by_type_name_sensitive() {
         "#
     ));
 
-    let json_output = r#"[{"name": "C","type": "Dir"},{"name": "a.txt","type": "File"},{"name": "B.txt","type": "File"}]"#;
+    let json_output = r#"[{"name": "C","type": "Dir"},{"name": "B.txt","type": "File"},{"name": "a.txt","type": "File"}]"#;
     assert_eq!(actual.out, json_output);
 }
 

src/tests/test_strings.rs

@@ -89,3 +89,19 @@ fn single_tick_interpolation() -> TestResult {
 fn detect_newlines() -> TestResult {
     run_test("'hello\r\nworld' | lines | get 0 | str length", "5")
 }
+
+#[test]
+fn case_insensitive_sort() -> TestResult {
+    run_test(
+        r#"[a, B, d, C, f] | sort-by -i | to json --raw"#,
+        "[\"a\",\"B\",\"C\",\"d\",\"f\"]",
+    )
+}
+
+#[test]
+fn case_insensitive_sort_columns() -> TestResult {
+    run_test(
+        r#"[[version, package]; ["two", "Abc"], ["three", "abc"], ["four", "abc"]] | sort-by -i package version | to json --raw"#,
+        r#"[{"version": "four","package": "abc"},{"version": "three","package": "abc"},{"version": "two","package": "Abc"}]"#,
+    )
+}