-
Notifications
You must be signed in to change notification settings - Fork 326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use new Enso Hash Codes and Comparable #6060
Changes from 11 commits
491a988
3840517
314b152
410cbdc
90489c9
9dc5ae0
ddba647
bf622c3
8b7e895
53e3471
7ff0f48
20abf27
dfbae3b
0b1402f
4c975ea
55c2b3a
52b6aac
45af32b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,9 @@ | ||
import project.Data.Numbers.Integer | ||
|
||
from project.Data.Ordering import all | ||
from project.Data.Boolean import Boolean, True, False | ||
|
||
|
||
polyglot java import java.time.DayOfWeek | ||
|
||
type Day_Of_Week | ||
|
@@ -34,8 +37,9 @@ type Day_Of_Week | |
Day_Of_Week.Friday -> 5 | ||
Day_Of_Week.Saturday -> 6 | ||
|
||
shifted = if first_day == Day_Of_Week.Sunday then day_number else | ||
(day_number + 7 - (first_day.to_integer start_at_zero=True)) % 7 | ||
shifted = case first_day of | ||
Day_Of_Week.Sunday -> day_number | ||
_ -> (day_number + 7 - (first_day.to_integer start_at_zero=True)) % 7 | ||
|
||
shifted + if start_at_zero then 0 else 1 | ||
|
||
|
@@ -49,3 +53,14 @@ type Day_Of_Week | |
Day_Of_Week.Thursday -> DayOfWeek.THURSDAY | ||
Day_Of_Week.Friday -> DayOfWeek.FRIDAY | ||
Day_Of_Week.Saturday -> DayOfWeek.SATURDAY | ||
|
||
## PRIVATE | ||
type Day_Of_Week_Comparator | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks exactly as I have envisioned that when dreaming about |
||
compare x y = | ||
x_int = x.to_integer | ||
y_int = y.to_integer | ||
Comparable.from x_int . compare x_int y_int | ||
|
||
hash x = x.to_integer | ||
|
||
Comparable.from (_:Day_Of_Week) = Day_Of_Week_Comparator |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
from Standard.Base import all | ||
import Standard.Base.Data.Array_Proxy.Array_Proxy | ||
import Standard.Base.Data.Ordering.Comparator | ||
import Standard.Base.Errors.Common.Index_Out_Of_Bounds | ||
import Standard.Base.Errors.Illegal_Argument.Illegal_Argument | ||
import Standard.Base.Errors.Illegal_State.Illegal_State | ||
|
@@ -1258,15 +1257,23 @@ type Column | |
my_compare a b = Ordering.compare a.abs b.abs | ||
Examples.decimal_column.sort by=my_compare | ||
sort : Sort_Direction -> Boolean -> (Any -> Any -> Ordering) | Nothing -> Column | ||
sort self order=Sort_Direction.Ascending missing_last=True by=Nothing = | ||
order_bool = case order of | ||
Sort_Direction.Ascending -> True | ||
Sort_Direction.Descending -> False | ||
java_cmp = Comparator.new by | ||
rule = OrderBuilder.OrderRule.new self.java_column java_cmp order_bool missing_last | ||
mask = OrderBuilder.buildOrderMask [rule].to_array | ||
new_col = self.java_column.applyMask mask | ||
Column.Value new_col | ||
sort self order=Sort_Direction.Ascending missing_last=True by=Nothing = case by of | ||
Nothing -> | ||
order_bool = case order of | ||
Sort_Direction.Ascending -> True | ||
Sort_Direction.Descending -> False | ||
rule = OrderBuilder.OrderRule.new self.java_column order_bool missing_last | ||
mask = OrderBuilder.buildOrderMask [rule].to_array | ||
new_col = self.java_column.applyMask mask | ||
Column.Value new_col | ||
_ -> | ||
wrapped a b = case a of | ||
Nothing -> if b.is_nothing then Ordering.Equal else if missing_last then Ordering.Greater else Ordering.Less | ||
_ -> case b of | ||
Nothing -> if missing_last then Ordering.Less else Ordering.Greater | ||
_ -> by a b | ||
Comment on lines
+1270
to
+1274
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels like it could be a separate method that will be useful in other places: |
||
sorted = self.to_vector.sort order by=wrapped | ||
Column.from_vector self.name sorted | ||
|
||
## Creates a new Column with the specified range of rows from the input | ||
Column. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,73 +1,69 @@ | ||
package org.enso.base; | ||
|
||
import org.graalvm.polyglot.Context; | ||
import org.graalvm.polyglot.Value; | ||
|
||
import java.time.LocalDate; | ||
import java.time.LocalTime; | ||
import java.time.ZonedDateTime; | ||
import java.util.Comparator; | ||
import java.util.Locale; | ||
import java.util.function.BiFunction; | ||
import java.util.function.Function; | ||
import java.util.function.BiFunction; | ||
|
||
public class ObjectComparator implements Comparator<Object> { | ||
private static ObjectComparator INSTANCE; | ||
|
||
/** | ||
* A singleton instance of an ObjectComparator. | ||
* | ||
* @param fallbackComparator this MUST be the default .compare_to function for Enso. Needs to be | ||
* passed to allow calling back from Java. | ||
* @return Comparator object. | ||
*/ | ||
public static ObjectComparator getInstance(Function<Object, Function<Object, Value>> fallbackComparator) { | ||
if (INSTANCE == null) { | ||
INSTANCE = new ObjectComparator(fallbackComparator); | ||
public static final ObjectComparator DEFAULT = new ObjectComparator(); | ||
|
||
private static BiFunction<Object, Object, Integer> EnsoCompareCallback = null; | ||
jdunkerley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
private static void initCallbacks() { | ||
if (EnsoCompareCallback == null) { | ||
var module = Context.getCurrent().getBindings("enso").invokeMember("get_module", "Standard.Base.Data.Ordering"); | ||
var type = module.invokeMember("get_type", "Comparable"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This kind of poking around in Enso doesn't make me particularly happy. However the only alternative I can offer is to always make sure each call from Enso to Java (that needs this callback) passes the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also don't like such a poking in Enso. But I guess it is OK for this PR, as a temporary solution. |
||
|
||
var are_equal = module.invokeMember("get_method", type, "compare_callback"); | ||
EnsoCompareCallback = (v, u) -> { | ||
var result = are_equal.execute(null, v, u); | ||
if (result.isNull()) { | ||
throw new CompareException(u, v); | ||
} else { | ||
return result.asInt(); | ||
} | ||
}; | ||
} | ||
|
||
return INSTANCE; | ||
} | ||
|
||
private final Function<Object, Function<Object, Value>> fallbackComparator; | ||
private final Function<String, Function<String, Value>> textComparator; | ||
|
||
|
||
public ObjectComparator() { | ||
this( | ||
(a) -> (b) -> { | ||
throw new CompareException(a, b); | ||
}); | ||
public static int ensoCompare(Object value, Object other) throws ClassCastException { | ||
jdunkerley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
initCallbacks(); | ||
return EnsoCompareCallback.apply(value, other); | ||
} | ||
|
||
public ObjectComparator(Function<Object, Function<Object, Value>> fallbackComparator) { | ||
this(fallbackComparator, (a) -> (b) -> Value.asValue(Text_Utils.compare_normalized(a, b))); | ||
} | ||
private final BiFunction<String, String, Integer> textComparator; | ||
|
||
private ObjectComparator(Function<Object, Function<Object, Value>> fallbackComparator, Function<String, Function<String, Value>> textComparator) { | ||
this.fallbackComparator = fallbackComparator; | ||
this.textComparator = textComparator; | ||
public ObjectComparator() { | ||
this(true, Locale.ROOT); | ||
} | ||
|
||
/** | ||
* Create a copy of the ObjectComparator with case-insensitive text comparisons. | ||
* @param locale to use for case folding. | ||
* @return Comparator object. | ||
*/ | ||
public ObjectComparator withCaseInsensitivity(Locale locale) { | ||
return new ObjectComparator(this.fallbackComparator, (a) -> (b) -> Value.asValue(Text_Utils.compare_normalized_ignoring_case(a, b, locale))); | ||
public ObjectComparator(boolean caseSensitive, Locale locale) { | ||
if (caseSensitive) { | ||
textComparator = Text_Utils::compare_normalized; | ||
} else { | ||
textComparator = (a, b) -> Text_Utils.compare_normalized_ignoring_case(a, b, locale); | ||
} | ||
} | ||
|
||
/** | ||
* Create a copy of the ObjectComparator with case-insensitive text comparisons. | ||
* @param textComparator custom comparator for Text. | ||
* @return Comparator object. | ||
*/ | ||
public ObjectComparator withCustomTextComparator(Function<String, Function<String, Value>> textComparator) { | ||
return new ObjectComparator(this.fallbackComparator, textComparator); | ||
public ObjectComparator(Function<Object, Function<Object, Value>> textComparator) { | ||
this.textComparator = (a, b) -> { | ||
var result = textComparator.apply(a).apply(b); | ||
if (result.isNull()) { | ||
throw new CompareException(a, b); | ||
} | ||
return result.asInt(); | ||
}; | ||
} | ||
|
||
@Override | ||
public int compare(Object thisValue, Object thatValue) throws ClassCastException { | ||
public int compare(Object thisValue, Object thatValue) { | ||
// NULLs | ||
if (thisValue == null) { | ||
if (thatValue != null) { | ||
Comment on lines
-70
to
94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally, this should also be removed and delegate to some common method in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code delegates to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand? This is just a workaround that should be replaced with a proper solution. The workaround uses a Context, because it is its way to 'access' the current implementation and delegate to it (through a slow Java-to-Enso call). The whole idea of extracting the shared code is so that the logic for computing the hash for particular primitive types can be exposed in pure Java, so that our library could call into it directly in Java, without the overhead of an Java-to-Enso call. That is what is described in #5259, although maybe I did not describe this well enough. If it's not clear - let's discuss. |
||
|
@@ -121,7 +117,7 @@ public int compare(Object thisValue, Object thatValue) throws ClassCastException | |
|
||
// Text | ||
if (thisValue instanceof String thisString && thatValue instanceof String thatString) { | ||
return convertComparatorResult(textComparator.apply(thisString).apply(thatString), thisString, thatString); | ||
return this.textComparator.apply(thisString, thatString); | ||
} | ||
|
||
// DateTimes | ||
|
@@ -145,14 +141,6 @@ public int compare(Object thisValue, Object thatValue) throws ClassCastException | |
} | ||
|
||
// Fallback to Enso | ||
return convertComparatorResult(fallbackComparator.apply(thisValue).apply(thatValue), thisValue, thatValue); | ||
} | ||
|
||
private static int convertComparatorResult(Value comparatorResult, Object leftOperand, Object rightOperand) { | ||
if (comparatorResult.isNumber() && comparatorResult.fitsInInt()) { | ||
return comparatorResult.asInt(); | ||
} else { | ||
throw new CompareException(leftOperand, rightOperand); | ||
} | ||
return ensoCompare(thisValue, thatValue); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw. won't just that work? I'm not sure if we have to special-case the Sunday given that we do
%
later anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stack overflow - to_integer would be called each time,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the culprit is #6065.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right! Okay then let's keep it.
I guess it would have worked if we did:
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This was only changed to avoid having
==
in it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was just wondering on a side if we could further simplify it, removing the possibility of the infinite loop altogether.