From 3fa7403be07de12c41c39d4db5e50d62fa129657 Mon Sep 17 00:00:00 2001 From: Pavel Marek Date: Mon, 20 Mar 2023 19:16:14 +0100 Subject: [PATCH] Implement sort handling for different comparators --- .../Base/0.0.0-dev/src/Data/Vector.enso | 40 ++++++--- .../builtin/ordering/SortVectorNode.java | 45 ++++++---- test/Tests/src/Data/Ordering_Spec.enso | 89 ++++++++++++++++--- 3 files changed, 129 insertions(+), 45 deletions(-) diff --git a/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso b/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso index 58036210cb13..3f0a90d18303 100644 --- a/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso +++ b/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso @@ -18,11 +18,14 @@ import project.Error.Error import project.Error.Illegal_Argument.Illegal_Argument import project.Function.Function import project.Math +import project.Meta import project.Nothing.Nothing import project.Panic.Panic import project.Random import project.Warning.Warning +import project.IO + # We have to import also conversion methods, therefore, we import all from the Ordering # module from project.Data.Ordering import all @@ -873,9 +876,9 @@ type Vector a This case is handled by first grouping the `self` vector into groups with the same comparator, recursively sorting these groups, and then merging them back together. The order of the sorted groups in the - resulting vector is based on the order of elements' comparators in the - `self` vector, with the exception of the group for the default - comparator, which is always the first group. + resulting vector is based on the order of fully qualified names of + the comparators in the `self` vector, with the exception of the group + for the default comparator, which is always the first group. Additionally, a warning will be attached, explaining that incomparable values were encountered. @@ -912,17 +915,18 @@ type Vector a elems = if on == Nothing then self else self.map it-> on it elems.sort_builtin order.to_sign False -> + # The "default value" of `by` parameter is `Ordering.compare _ _`, but because + # there is no function equality, the real default value is Nothing. + by_non_null = if by == Nothing then (Ordering.compare _ _) else by # Groups of elements with different comparators - groups = comps.distinct.map comp-> + groups = comps.map comp-> self.filter it-> Comparable.from (on it) == comp - # TODO: Runtime.assert groups.reduce 0 acc->it-> acc + it.length == self.length case groups.length of - # self consists only of elements with the same comparator. + # self consists only of elements with the same comparator, that + # is not `Default_Comparator`. # Forward to Array.sort 1 -> - # The default value of `by` parameter is `Ordering.compare _ _` - by_non_null = if by == Nothing then (Ordering.compare _ _) else by comp_ascending l r = by_non_null (on l) (on r) comp_descending l r = by_non_null (on r) (on l) compare = if order == Sort_Direction.Ascending then comp_ascending else @@ -931,11 +935,21 @@ type Vector a if new_vec_arr.is_error then Error.throw new_vec_arr else Vector.from_polyglot_array new_vec_arr _ -> - # Recurse on each group, and attach a warning - # TODO: Sort Default_Comparator group as the first one? - sorted_groups = groups.map it-> it.sort order on by - comparators_text = comps.distinct.to_text - Warning.attach ("Different comparators: " + comparators_text) sorted_groups.flatten + # Partition into groups sorted by FQN of their comparator's name, + # with the default comparator as the first group. + groups_with_comps_sorted_by_fqn = groups.zip comps . sort by=pair_1->pair_2-> + comp_1 = pair_1.last + comp_2 = pair_2.last + if comp_1 == Default_Comparator then Ordering.Less else + if comp_2 == Default_Comparator then Ordering.Greater else + comp_1_fqn = Meta.get_qualified_type_name comp_1 + comp_2_fqn = Meta.get_qualified_type_name comp_2 + Ordering.compare comp_1_fqn comp_2_fqn + # Recurse on each group + sorted_groups = groups_with_comps_sorted_by_fqn.map it-> it.first.sort order on by + # Merge the groups and attach a warning + comparators_warn_text = groups_with_comps_sorted_by_fqn.map (it-> it.last) . to_text + Warning.attach ("Different comparators: " + comparators_warn_text) sorted_groups.flatten ## UNSTABLE Keeps only unique elements within the Vector, removing any duplicates. diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/ordering/SortVectorNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/ordering/SortVectorNode.java index 525616482a13..fe5ea13a988b 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/ordering/SortVectorNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/ordering/SortVectorNode.java @@ -1,5 +1,7 @@ package org.enso.interpreter.node.expression.builtin.ordering; +import com.oracle.truffle.api.CompilerDirectives; +import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary; import com.oracle.truffle.api.dsl.Cached; import com.oracle.truffle.api.dsl.GenerateUncached; import com.oracle.truffle.api.dsl.Specialization; @@ -8,7 +10,10 @@ import com.oracle.truffle.api.interop.UnsupportedMessageException; import com.oracle.truffle.api.library.CachedLibrary; import com.oracle.truffle.api.nodes.Node; +import com.oracle.truffle.api.profiles.BranchProfile; import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; import org.enso.interpreter.dsl.AcceptsError; import org.enso.interpreter.dsl.BuiltinMethod; import org.enso.interpreter.node.expression.builtin.interop.syntax.HostValueToEnsoNode; @@ -24,7 +29,6 @@ import org.enso.interpreter.runtime.error.Warning; import org.enso.interpreter.runtime.error.WarningsLibrary; import org.enso.interpreter.runtime.error.WithWarnings; -import org.enso.interpreter.runtime.util.Collections.ArrayListObj; /** * Sorts a vector with elements that have only Default_Comparator, thus, only elements with a @@ -63,6 +67,7 @@ Object sortCached(Object self, long ascending, @Cached HostValueToEnsoNode hostValueToEnsoNode, @Cached TypeOfNode typeOfNode, @Cached AnyToTextNode toTextNode, + @Cached BranchProfile warningEncounteredProfile, @CachedLibrary(limit = "10") InteropLibrary interop, @CachedLibrary(limit = "5") WarningsLibrary warningsLib) { EnsoContext ctx = EnsoContext.get(this); @@ -93,13 +98,15 @@ Object sortCached(Object self, long ascending, Arrays.sort(elems, comparator); var vector = Vector.fromArray(new Array(elems)); - // Check for the warnings attached from the Comparator - Warning[] currWarns = null; if (comparator.encounteredWarnings()) { - currWarns = (Warning[]) comparator.getWarnings(); - } - if (currWarns != null) { - return WithWarnings.appendTo(vector, new ArrayRope<>(currWarns)); + warningEncounteredProfile.enter(); + CompilerDirectives.transferToInterpreter(); + Warning[] warns = comparator.getWarnings() + .stream() + .map(Text::create) + .map(text -> Warning.create(EnsoContext.get(this), text, this)) + .toArray(Warning[]::new); + return WithWarnings.appendTo(vector, new ArrayRope<>(warns)); } else { return vector; } @@ -135,7 +142,12 @@ else if (type == builtins.dateTime()) { } else if (type == builtins.duration()) { return 6; - } else { + } + else if (type == builtins.vector()) { + // vectors are incomparable, but we want to sort them before Nothings and NaNs. + return 50; + } + else { throw new IllegalStateException("Unexpected type: " + type); } } @@ -159,7 +171,7 @@ private final class Comparator implements java.util.Comparator { private final TypeOfNode typeOfNode; private final AnyToTextNode toTextNode; private final boolean ascending; - private final ArrayListObj warnings = new ArrayListObj<>(); + private final Set warnings = new HashSet<>(); private Comparator(LessThanNode lessThanNode, EqualsNode equalsNode, TypeOfNode typeOfNode, AnyToTextNode toTextNode, boolean ascending) { @@ -205,23 +217,20 @@ private int compareTypes(Object x, Object y) { return ascending ? res : -res; } + @TruffleBoundary private void attachIncomparableValuesWarning(Object x, Object y) { var xStr = toTextNode.execute(x).toString(); var yStr = toTextNode.execute(y).toString(); - var payload = Text.create("Values " + xStr + " and " + yStr + " are incomparable"); - var sortNode = SortVectorNode.this; - var warn = Warning.create(EnsoContext.get(sortNode), payload, sortNode); - warnings.add(warn); + String warnText = "Values " + xStr + " and " + yStr + " are incomparable"; + warnings.add(warnText); } private boolean encounteredWarnings() { - return warnings.size() > 0; + return !warnings.isEmpty(); } - private Object[] getWarnings() { - Warning[] warns = new Warning[warnings.size()]; - warns = warnings.toArray(warns.getClass()); - return warns; + private Set getWarnings() { + return warnings; } } } diff --git a/test/Tests/src/Data/Ordering_Spec.enso b/test/Tests/src/Data/Ordering_Spec.enso index 3be7f830507a..7e66431e948c 100644 --- a/test/Tests/src/Data/Ordering_Spec.enso +++ b/test/Tests/src/Data/Ordering_Spec.enso @@ -16,7 +16,18 @@ type Ord_Comparator Comparable.from (_:Ord) = Ord_Comparator -## Unordered pair + +type My_Type + Value val + +type My_Type_Comparator + compare x y = (Comparable.from x.val).compare x.val y.val + hash x = (Comparable.from x.val).hash x.val + +Comparable.from (_:My_Type) = My_Type_Comparator + + +## Unordered pair - its `compare` method returns either `Nothing` or `Ordering.Equal`. type UPair Value x y @@ -98,7 +109,7 @@ spec = Ordering.compare Ordering.Less Nothing . should_fail_with Type_Error Ordering.compare Ordering.Less "Hello" . should_fail_with Type_Error - Test.group "Sorting" <| + Test.group "Sorting with the default comparator" <| ## Expects that `result` contains incomparable values warning. The values within the warning message can be switched - the order does not matter. Iterates through all the warnings of result. @@ -112,6 +123,16 @@ spec = has_expected_warning = Warning.get_all result . map (_.value) . any (it-> it == expected_warn_msg_left || it == expected_warn_msg_right) has_expected_warning . should_be_true + # Same as `expect_incomparable_warn`, but does not check for a particular + # warning message + expect_some_incomparable_warn result = + expected_pattern = "Values .+ and .+ are incomparable" + has_expected_warning = Warning.get_all result . map (_.value) . any (it-> ) TODO... + + expect_no_warns : Any -> Nothing + expect_no_warns result = + Warning.get_all result . length . should_equal 0 + Test.specify "should be able to sort primitive types" <| [3, 2, 1, Nothing].sort . should_equal [1, 2, 3, Nothing] [Nothing, Number.nan].sort . at 0 . is_nan . should_be_true @@ -128,6 +149,26 @@ spec = [3, True, 2, False].sort . should_equal [2, 3, False, True] [Nothing, False].sort . should_equal [False, Nothing] + Test.specify "should be able to sort any single-element vector without any warnings" <| + [Nothing].sort . should_equal [Nothing] + expect_no_warns [Nothing].sort + [[Nothing]].sort . should_equal [[Nothing]] + expect_no_warns [[Nothing]].sort + [Number.nan].sort . should_equal [Number.nan] + expect_no_warns [Nothing].sort + [[Number.nan]].sort . should_equal [[Number.nan]] + expect_no_warns [[Number.nan]].sort + + Test.specify "should produce warnings when sorting nested vectors" <| + [[1], [2]].sort . should_equal [[1], [2]] + [[2], [1]].sort . should_equal [[2], [1]] + + Test.specify "should be able to sort primitive values in atoms" <| + [Ord.Value Number.nan, Ord.Value 20, Ord.Value 10].sort . should_equal [Ord.Value 10, Ord.Value 20, Ord.Value Number.nan] + + Test.specify "should produce warnings when sorting primitive values in atoms" <| + expect_incomparable_warn 1 Number.nan [Ord.Value 1, Ord.Value Number.nan].sort + Test.specify "should attach warning when trying to sort incomparable values" <| expect_incomparable_warn Nothing Number.nan <| [Nothing, Number.nan].sort expect_incomparable_warn 1 "hello" <| [1, "hello"].sort @@ -142,22 +183,42 @@ spec = expect_incomparable_warn 1 Number.nan [1, Warning.attach "my_warn" Number.nan].sort Problems.expect_warning "my_warn" <| [1, Warning.attach "my_warn" Number.nan].sort . at 1 - Test.specify "should be able to sort types with different comparators" <| - [Ord.Value 4, Ord.Value 3, 20, 10].sort . should_equal [Ord.Value 3, Ord.Value 4, 10, 20] - [Ord.Value 4, 20, Ord.Value 3, 10].sort . should_equal [Ord.Value 3, Ord.Value 4, 10, 20] + Test.group "Sorting with multiple comparators" <| + Test.specify "should sort primitive values with the default comparator as the first group" <| + [Ord.Value 4, Ord.Value 3, 20, 10].sort . should_equal [10, 20, Ord.Value 3, Ord.Value 4] + [Ord.Value 4, 20, Ord.Value 3, 10].sort . should_equal [10, 20, Ord.Value 3, Ord.Value 4] [20, Ord.Value 4, Ord.Value 3, 10].sort . should_equal [10, 20, Ord.Value 3, Ord.Value 4] - [Ord.Value 4, 20, Ord.Value 3, 10].sort . should_equal [Ord.Value 3, Ord.Value 4, 10, 20] - [Nothing, Ord.Value 4, 20, Ord.Value 3, 10].sort . should_equal [Ord.Value 3, Ord.Value 4, 10, 20, Nothing] + [Ord.Value 4, 20, Ord.Value 3, 10].sort . should_equal [10, 20, Ord.Value 3, Ord.Value 4] + [Nothing, Ord.Value 4, 20, Ord.Value 3, 10].sort . should_equal [10, 20, Nothing, Ord.Value 3, Ord.Value 4] + [Ord.Value 4, 20, Ord.Value 3, Nothing, 10].sort . should_equal [10, 20, Nothing, Ord.Value 3, Ord.Value 4] Test.specify "should produce warning when sorting types with different comparators" <| [Ord.Value 1, 1].sort . should_equal [Ord.Value 1, 1] - Problems.expect_warning "Different comparators: [Ord_Comparator, Default_Comparator]" <| [Ord.Value 1, 1].sort - - Test.specify "should be able to sort primitive values in atoms" <| - [Ord.Value Number.nan, Ord.Value 20, Ord.Value 10].sort . should_equal [Ord.Value 10, Ord.Value 20, Ord.Value Number.nan] - - Test.specify "should produce warnings when sorting primitive values in atoms" <| - expect_incomparable_warn 1 Number.nan [Ord.Value 1, Ord.Value Number.nan].sort + Problems.expect_warning "Different comparators: [Default_Comparator, Ord_Comparator]" <| [Ord.Value 1, 1].sort + + Test.specify "should merge groups of values with custom comparators based on the comparators FQN" <| + [Ord.Value 1, My_Type.Value 1].sort . should_equal [My_Type.Value 1, Ord.Value 1] + Problems.expect_warning "Different comparators: [My_Type_Comparator, Ord_Comparator]" <| [Ord.Value 1, My_Type.Value 1].sort + + Test.specify "should be stable when sorting values with different comparators" <| + [Ord.Value 1, 20, My_Type.Value 1, 10].sort . should_equal [10, 20, My_Type.Value 1, Ord.Value 1] + [20, Ord.Value 1, My_Type.Value 1, 10].sort . should_equal [10, 20, My_Type.Value 1, Ord.Value 1] + [20, My_Type.Value 1, Ord.Value 1, 10].sort . should_equal [10, 20, My_Type.Value 1, Ord.Value 1] + [20, 10, My_Type.Value 1, Ord.Value 1].sort . should_equal [10, 20, My_Type.Value 1, Ord.Value 1] + [My_Type.Value 1, Ord.Value 1, 20, 10].sort . should_equal [10, 20, My_Type.Value 1, Ord.Value 1] + [Ord.Value 1, 20, 10, My_Type.Value 1].sort . should_equal [10, 20, My_Type.Value 1, Ord.Value 1] + + # In other words, if there are incomparable values, make sure that warning for + # "incomparable value X Y" is attached just once. + Test.specify "should not duplicate warnings" <| + Problems.get_attached_warnings ([Nothing, (Ord.Value 1), 20].sort) . should_equal ["Different comparators: [Default_Comparator, Ord_Comparator]", "Values 20 and Nothing are incomparable"] + + Test.specify "should be able to sort even unordered values" pending="https://github.com/enso-org/enso/issues/5834" <| + [Ord.Value 2, UPair.Value "a" "b", Ord.Value 1, UPair.Value "c" "d"].sort . should_equal [Ord.Value 2, Ord.Value 1, UPair.Value "a" "b", UPair.Value "c" "d"] + [Ord.Value 2, UPair.Value "X" "Y", Ord.Value 1, UPair.Value "c" "d"].sort . should_equal [Ord.Value 2, Ord.Value 1, UPair.Value "X" "Y", UPair.Value "c" "d"] + + Test.specify "should produce warning when sorting unordered values" pending="https://github.com/enso-org/enso/issues/5834" <| + expect_incomparable_warn (UPair.Value 1 2) (UPair.Value 3 4) [UPair.Value 1 2, UPair.Value 3 4].sort