diff --git a/main/src/com/google/refine/expr/functions/arrays/Sort.java b/main/src/com/google/refine/expr/functions/arrays/Sort.java index e0f0436ed..2476d5dc1 100644 --- a/main/src/com/google/refine/expr/functions/arrays/Sort.java +++ b/main/src/com/google/refine/expr/functions/arrays/Sort.java @@ -35,6 +35,7 @@ package com.google.refine.expr.functions.arrays; import java.util.Arrays; import java.util.Collections; +import java.util.Comparator; import java.util.List; import java.util.Properties; @@ -47,29 +48,34 @@ import com.google.refine.util.JSONUtilities; public class Sort implements Function { @Override - @SuppressWarnings("unchecked") + @SuppressWarnings({ "unchecked", "rawtypes" }) public Object call(Properties bindings, Object[] args) { if (args.length == 1) { Object v = args[0]; - if (v != null) { if (v.getClass().isArray()) { Object[] a = (Object[]) v; - Object[] r = a.clone(); - - Arrays.sort(r, 0, r.length); - + Comparable[] r = new Comparable[a.length]; + for (int i = 0; i < r.length; i++) { + if (a[i] instanceof Comparable) { + r[i] = (Comparable) a[i]; + } else if (a[i] == null) { + r[i] = null; + } else { + return new EvalError(ControlFunctionRegistry.getFunctionName(this) + " expects an array of uniform type"); + } + } + Arrays.sort(r, Comparator.nullsLast(Comparator.naturalOrder())); return r; } else if (v instanceof ArrayNode) { - Object[] r = JSONUtilities.toArray((ArrayNode) v); - - Arrays.sort(r, 0, r.length); - + // FIXME: Probably need a test for this + // Comparable[] is a (slight) lie since nulls can be included, but our comparator will handle them + Comparable[] r = (Comparable[]) JSONUtilities.toSortableArray((ArrayNode) v); + Arrays.sort(r, Comparator.nullsLast(Comparator.naturalOrder())); return r; } else if (v instanceof List) { List> a = (List>) v; - Collections.sort(a); - + Collections.sort(a, Comparator.nullsLast(Comparator.naturalOrder())); return a; } } @@ -84,7 +90,7 @@ public class Sort implements Function { @Override public String getParams() { - return "array a"; + return "array a of uniform type"; } @Override diff --git a/main/src/com/google/refine/expr/util/JsonValueConverter.java b/main/src/com/google/refine/expr/util/JsonValueConverter.java index dde09c32f..06e3674b6 100644 --- a/main/src/com/google/refine/expr/util/JsonValueConverter.java +++ b/main/src/com/google/refine/expr/util/JsonValueConverter.java @@ -35,29 +35,55 @@ import com.fasterxml.jackson.databind.JsonNode; */ public class JsonValueConverter { - public static Object convert(JsonNode value) { - if (value == null) { - return null; - } - if (value.isObject()) { - return value; - } else if (value.isArray()) { - return value; - } else if (value.isBigDecimal() || value.isDouble() || value.isFloat()) { - return value.asDouble(); - } else if (value.isBigInteger()) { - return value.asLong(); - } else if (value.isInt()) { - return value.asInt(); - } else if (value.isBinary() || value.isTextual()) { - return value.asText(); - } else if (value.isBoolean()) { - return value.asBoolean(); - } else if (value.isNull()) { - return null; - } else { - return null; - } - } + public static Object convert(JsonNode value) { + if (value == null) { + return null; + } + if (value.isObject()) { + return value; + } else if (value.isArray()) { + return value; + } else if (value.isBigDecimal() || value.isDouble() || value.isFloat()) { + return value.asDouble(); + } else if (value.isBigInteger()) { + return value.asLong(); + } else if (value.isInt()) { + return value.asInt(); + } else if (value.isBinary() || value.isTextual()) { + return value.asText(); + } else if (value.isBoolean()) { + return value.asBoolean(); + } else if (value.isNull()) { + return null; + } else { + return null; + } + } + + @SuppressWarnings("rawtypes") + public static Comparable convertComparable(JsonNode value) { + if (value == null) { + return null; + } + if (value.isContainerNode()) { + // TODO: return null instead (like fallthrough case) + throw new IllegalArgumentException("Arrays and objects aren't comparable"); + } else if (value.isBigDecimal() || value.isDouble() || value.isFloat()) { + return value.asDouble(); + } else if (value.isBigInteger()) { + return value.asLong(); + } else if (value.isInt()) { + return value.asInt(); + } else if (value.isBinary() || value.isTextual()) { + return value.asText(); + } else if (value.isBoolean()) { + return value.asBoolean(); + } else if (value.isNull()) { + return null; + } else { + return null; + } + } + } diff --git a/main/src/com/google/refine/util/JSONUtilities.java b/main/src/com/google/refine/util/JSONUtilities.java index adb0d106d..3acf5ab53 100644 --- a/main/src/com/google/refine/util/JSONUtilities.java +++ b/main/src/com/google/refine/util/JSONUtilities.java @@ -182,4 +182,17 @@ public class JSONUtilities { } return result; } + + @SuppressWarnings("rawtypes") + public static Comparable[] toSortableArray(ArrayNode v) { + if (v == null) { + return null; + } + Comparable[] result = new Comparable[v.size()]; + for (int i = 0; i != v.size(); i++) { + result[i] = JsonValueConverter.convertComparable(v.get(i)); + } + return result; + } + } diff --git a/main/tests/server/src/com/google/refine/RefineTest.java b/main/tests/server/src/com/google/refine/RefineTest.java index b7cbb6374..99c1c900a 100644 --- a/main/tests/server/src/com/google/refine/RefineTest.java +++ b/main/tests/server/src/com/google/refine/RefineTest.java @@ -350,6 +350,20 @@ public class RefineTest extends PowerMockTestCase { Assert.assertEquals(result.toString(), test[1], "Wrong result for expression: " + test[0]); } + /** + * Parse and evaluate a GREL expression and compare the result an expected + * type using instanceof + * + * @param bindings + * @param test + * @throws ParsingException + */ + protected void parseEvalType(Properties bindings, String test, @SuppressWarnings("rawtypes") Class clazz) + throws ParsingException { + Evaluable eval = MetaParser.parse("grel:" + test); + Object result = eval.evaluate(bindings); + Assert.assertTrue(clazz.isInstance(result), "Wrong result type for expression: " + test); + } @AfterMethod public void TearDown() throws Exception { diff --git a/main/tests/server/src/com/google/refine/expr/functions/arrays/SortTests.java b/main/tests/server/src/com/google/refine/expr/functions/arrays/SortTests.java index 561f84fb1..ff8b62e25 100644 --- a/main/tests/server/src/com/google/refine/expr/functions/arrays/SortTests.java +++ b/main/tests/server/src/com/google/refine/expr/functions/arrays/SortTests.java @@ -28,13 +28,51 @@ package com.google.refine.expr.functions.arrays; import org.testng.annotations.Test; +import com.google.refine.RefineTest; +import com.google.refine.expr.EvalError; +import com.google.refine.expr.ParsingException; import com.google.refine.util.TestUtils; -public class SortTests { +public class SortTests extends RefineTest { @Test public void serializeSort() { - String json = "{\"description\":\"Sorts array a\",\"params\":\"array a\",\"returns\":\"array\"}"; + String json = "{\"description\":\"Sorts array a\",\"params\":\"array a of uniform type\",\"returns\":\"array\"}"; TestUtils.isSerializedTo(new Sort(), json); } + + @Test + public void sortJsonArray() throws ParsingException { + String[] test = {"'[2,1,3]'.parseJson().sort().toString()", "[1, 2, 3]"}; + parseEval(bindings, test); + String[] test1 = {"'[2,null,3]'.parseJson().sort().toString()", "[2, 3, null]"}; + parseEval(bindings, test1); + } + + @Test + public void sortArray() throws ParsingException { + String[] test = {"[2,1,3].sort().toString()", "[1, 2, 3]"}; + parseEval(bindings, test); + String[] test1 = {"[2,null,3].sort().toString()", "[2, 3, null]"}; + parseEval(bindings, test1); + + String[] test2 = {"['z','b','c','a'].sort().toString()", "[a, b, c, z]"}; + parseEval(bindings, test2); + String[] test3 = {"['z',null,'c','a'].sort().toString()", "[a, c, z, null]"}; + parseEval(bindings, test3); + + String[] test4 = {"[toDate(2020), '2018-03-02'.toDate()].sort().toString()", "[2018-03-02T00:00Z, 2020-01-01T00:00Z]"}; + parseEval(bindings, test4); + } + + public void sortMixedArray() throws ParsingException { + String test = "[2,1.0,3].sort().toString()"; + parseEvalType(bindings, test, EvalError.class); + test = "[2,'a',3].sort().toString()"; + parseEvalType(bindings, test, EvalError.class); + test = "'[2,\"a\",3]'.parseJson().sort().toString()"; + parseEvalType(bindings, test, EvalError.class); + + } + }