Skip to content

Commit

Permalink
Implement sort handling for different comparators
Browse files Browse the repository at this point in the history
  • Loading branch information
@Akirathan
Akirathan committed Mar 20, 2023
1 parent c6e673a commit 3fa7403
Show file tree
Hide file tree
Showing 3 changed files with 129 additions and 45 deletions.
40 changes: 27 additions & 13 deletions distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down
45 changes: 27 additions & 18 deletions ...e/src/main/java/org/enso/interpreter/node/expression/builtin/ordering/SortVectorNode.java
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
}
}
Expand All @@ -159,7 +171,7 @@ private final class Comparator implements java.util.Comparator<Object> {
private final TypeOfNode typeOfNode;
private final AnyToTextNode toTextNode;
private final boolean ascending;
private final ArrayListObj<Warning> warnings = new ArrayListObj<>();
private final Set<String> warnings = new HashSet<>();

private Comparator(LessThanNode lessThanNode, EqualsNode equalsNode, TypeOfNode typeOfNode,
AnyToTextNode toTextNode, boolean ascending) {
Expand Down Expand Up @@ -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<String> getWarnings() {
return warnings;
}
}
}
89 changes: 75 additions & 14 deletions test/Tests/src/Data/Ordering_Spec.enso
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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



Expand Down

0 comments on commit 3fa7403

Please sign in to comment.