JSON Improvements, small Table stuff, Statistic in Enso not Java and few other minor bits. #3964

jdunkerley · 2022-12-08T20:31:58Z

Pull Request Description

Aligned compare_to so returns Type_Error if that is wrong type for Text, Ordering and Duration.
Add empty_object, empty_array. get_or_else, at, field_names and length to Json.
Fix Json serialisation of NaN and Infinity (to "null").
Added length, at and to_vector to Pair (allowing it to be treated as a Vector).
Added running_fold to the Vector and Range.
Added first and last to the Vector.Builder.
Allow order_by to take a single Sort_Column or have a mix of Text and Sort_Column.Name in a Vector.
Allow select_columns_helper to take a Text value. Allows for a single field in group_by in cross_tab.
Added Patch and Custom to HTTP_Method.
Added running Statistic calculation and moved more of the logic from Java to Enso. Performance seems similar to pure Java version now.

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the
Scala,
Java,
and
Rust
style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH
  ./run ide build and ./run ide watch.

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Filter_Condition.enso

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Json.enso

radeusgd · 2022-12-14T11:31:46Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Json.enso

+    ## Gets the value associated with the given index in this object.
+
+       Arguments:
+       - index: The index position  from which to get the value.
+    at : Integer -> Json ! No_Such_Field | Out_Of_Bounds
+    at self index = case self of
+        Json.Array array -> array.at index
+        Json.String text -> Json.String (text.at index)
+        Json.Null -> Json.Null
+        _ -> Error.throw (Illegal_Argument.Error "Json.at: self must be a String or an Array.")


When can No_Such_Field be thrown here? Seems spurious.
Also, technically we should add Illegal_Argument here though. And the same for get and get_or_else.

Btw. should we allow get and at to be aliases of each other to emulate how in JS we use the same syntax for both: obj[field] and array[index]? Just a thought, although I guess keeping them separate is safer, so I'm probably for keeping it - but wanted to address possibilities.

Legacy from a previous step.
This is a partial step forward better JSON support is coming. The current is a disaster to work with.

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Json/Internal.enso

radeusgd · 2022-12-14T11:37:24Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Ordering.enso

+    compare_to self that = case that of
+        _ : Ordering -> self.to_sign.compare_to that.to_sign
+        _ -> Error.throw (Type_Error.Error Ordering that "that")


Btw. what yielded this change?

I'm fine with it but usually we do not check the input types anyway, but I'm wondering what led to this change and if maybe we can do a more general solution at Incomparable_Values.handle_errors level?

It should be a Type_Error and when I was reviewing compare_to spotted it.
If passed an invalid type for comparing we should be consistent and return a Type_Error.

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Pair.enso

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Range.enso

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Statistics.enso

radeusgd

Looked at most changes and tests and they seem good. I want to have a deeper look at the statistics though.

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Statistics.enso

radeusgd · 2022-12-14T11:54:54Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Statistics.enso

+
+        java_stats = statistics.map .order
+        skip_moments = java_stats.all s->s.is_nothing
+        moments = if skip_moments then Nothing else Moments.new (java_stats.filter v->v!=Nothing . fold 0 .max)


I wonder if we should allow 2.max Nothing == 2, then we'd be able to skip the filter step.

Don't want to change that behavior at the moment.

ok, fair enough

radeusgd · 2022-12-14T12:14:26Z

distribution/lib/Standard/Base/0.0.0-dev/src/Error/Common.enso

-    ## UNSTABLE
+    ## PRIVATE
+       UNSTABLE


This seems a bit wrong.

I think we should have a separate tag for marking entries that should not be shown in the GUI.

These errors are not meant to be private - normal text user should be able to use them. Well, even GUI user may want to value.catch Illegal_Argument.

If we mark stuff that is intended for usage by users as private, they will end up using it anyway and alongside it, they will be using the truly private stuff too.

Not something to address in this PR, but I really think we should separate truly private implementation details from stuff that is just meant to be hidden to the GUI. Ideally, before the release.

Otherwise, we will have no good way of saying 'this is an implementation detail, do not rely on it staying as it is'.

Agree the concept of genuinely private keeps coming up. Using the comment to mask is just a workaround.

Personally, I'm fine with the PRIVATE comment, although I see that a genuine private would be even better. Still, not good to mix our current only concept of privacy with a separate concept of 'users are totally FINE using this, but do not show in the GUI component browser'.

And it sounds like a simple thing to workaround, can we coordinate with IDE/SuggestionDatabase guys to just add one more keyword, like TEXT-ONLY or HIDDEN which would for now work the same way as PRIVATE (i.e. the modification in GUI / DB just needs to add an ||) and use it here? This way we'll avoid abusing PRIVATE and can do this properly before we get proper private (which probably won't happen too soon).

distribution/lib/Standard/Base/0.0.0-dev/src/Error/Common.enso

distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Table_Helpers.enso

test/Table_Tests/src/Common_Table_Operations/Order_By_Spec.enso

radeusgd · 2022-12-14T12:21:37Z

test/Table_Tests/src/In_Memory/Table_Spec.enso

+        Test.specify "should allow a grouping by text" <|
+            t = Table.new [["Group", ["A","B","A","B","A","B","A","B","A"]], ["Key", ["x", "x", "x", "x", "y", "y", "y", "z", "z"]], ["Value", [1, 2, 3, 4, 5, 6, 7, 8, 9]]]
+            t1 = t.cross_tab "Group" "Key"
+            t1.row_count . should_equal 2
+            t1.column_count . should_equal 4
+            t1.at "Group" . to_vector . should_equal ["A", "B"]
+            t1.at "x" . to_vector . should_equal [2, 2]
+            t1.at "y" . to_vector . should_equal [2, 1]
+            t1.at "z" . to_vector . should_equal [1, 1]


Maybe worth double-checking that the grouping supports Unicode normalization in backends with test_selection.supports_unicode_normalization == True?

test/Tests/src/Data/Json_Spec.enso

test/Tests/src/Data/Pair_Spec.enso

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Statistics.enso

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Json/Internal.enso

radeusgd · 2022-12-14T13:42:30Z

std-bits/base/src/main/java/org/enso/base/statistics/CountMinMax.java

-import java.util.Iterator;
-import java.util.function.BiFunction;
-import java.util.stream.Stream;
+import org.graalvm.polyglot.Value;

 public class CountMinMax {


I'm not sure if I understand why we keep that in Java?
Seems like it should be well emulated by a collection of 4 Refs.
Although I do like that it is encapsulated in a purposeful object - this is neat. Just wondering if the object couldn't be managed by Enso? Or were Refs not performant enough here?

Massively slower

Oh, ok then. interesting though as in principle it should have similar characteristics. Is it worth discussing with @JaroslavTulach if we want to find out why?

yes we will have a ticket to look at it

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Statistics.enso

hubertp · 2022-12-14T16:02:33Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Filter_Condition.enso

+         It accepts a Text value to check if the value contains it. In case of
+         Table operations, it can accept another column - then the corresponding
+         values from the source column and the provided column are checked.
+    Not_Contains (substring:Text)


s/Not_Contains/Missing?

Sounds nice, but the Not_ prefix is consistent with all the other operations so I'm not sure if consistency here will not be more important.

…nd length to Json. Added first and last to the Vector.Builder. Allow order_by to take a single Sort_Column ot have a mix of Text and Sort_Column in a Vector. Allow select_columns helper to take a Text value. Allows for single field in

Add new Statistic_Aggregator to replace CountMinMax and MomentStatistic.

Add tests for running_fold.

Fix Pair compilation error.

jdunkerley force-pushed the wip/jd/table-union-initial branch 5 times, most recently from ac2413b to c246a19 Compare December 13, 2022 23:37

jdunkerley marked this pull request as ready for review December 13, 2022 23:37

jdunkerley requested a review from radeusgd as a code owner December 13, 2022 23:37

jdunkerley requested review from hubertp and JaroslavTulach December 13, 2022 23:39

jdunkerley changed the title ~~JSON Improvements, Quality of life Table stuff and Table.Union~~ JSON Improvements, small Table stuff, Statistic in Enso not Java and few other minor bits. Dec 13, 2022

jdunkerley force-pushed the wip/jd/table-union-initial branch from c246a19 to 079f638 Compare December 14, 2022 07:03

radeusgd reviewed Dec 14, 2022

View reviewed changes

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Statistics.enso Outdated Show resolved Hide resolved

radeusgd reviewed Dec 14, 2022

View reviewed changes

hubertp approved these changes Dec 14, 2022

View reviewed changes

jdunkerley requested a review from 4e6 as a code owner December 14, 2022 16:35

jdunkerley added 13 commits December 14, 2022 16:38

Fix Table_Helpers.

f5905e4

Add some functions to Pair to make it work like a Vector.

31f2007

Ensure compare_to returns Type_Error if not correct type.

326d47a

Add running_fold.

e2680cd

Fix duration test to expect Type_Error.

850a01f

Add new Statistic_Aggregator to replace CountMinMax and MomentStatistic.

Add PATCH method.

695b3f1

Custom HTTP Method and new JSON Object parsed using JS

53f95e4

First pass at Enso Statistics Moment.

79b418a

Work on JSON_Object.

9fbe7d4

Add running Statistics prototype.

86e6546

Allow NaN/Infinity to be serialised as NULL for JSON.

2f7de63

Add tests for NaN and Inifite to_json.

1ef3b89

jdunkerley and others added 21 commits December 14, 2022 16:38

Add Not_Contains and tests.

a848841

Remove JSON_Object from this PR.

aec01e0

Add JSON and Ordering tests.

54b6bee

Test for Pair.

d3592f5

Undo

f0a5fd0

Add running_fold to Range.

6fc38e1

Add tests for running_fold.

Remove some old tests.

2225b82

Fix failing Table tests.

0fa5f58

Add transpose and cross_tab single field test.

f77714f

Fix Pair compilation error.

Remaining JSON tests.

555693d

Statistics tests and more in Enso less in Java.

d6972cb

Changelog and exports.

8935f44

Java Format and missing import.

3bea818

Failed test.

89e4e4d

Commit right file...

7e2c800

Some PR comments.

e4f36d8

Restructure stats based on PR.

a513d92

More PR Stuff...

e7c1bea

Fix Stats test.

2fa22e3

Exclude Pair as now has case of - constants.

3df925c

Fix failing Benchmark

4370f90

jdunkerley force-pushed the wip/jd/table-union-initial branch from 07448fc to 4370f90 Compare December 14, 2022 16:38

radeusgd approved these changes Dec 14, 2022

View reviewed changes

jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Dec 14, 2022

Merge branch 'develop' into wip/jd/table-union-initial

a7e854a

mergify bot merged commit 77fe69d into develop Dec 14, 2022

mergify bot deleted the wip/jd/table-union-initial branch December 14, 2022 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON Improvements, small Table stuff, Statistic in Enso not Java and few other minor bits. #3964

JSON Improvements, small Table stuff, Statistic in Enso not Java and few other minor bits. #3964

jdunkerley commented Dec 8, 2022 •

edited

Loading

radeusgd Dec 14, 2022

jdunkerley Dec 14, 2022

radeusgd Dec 14, 2022

jdunkerley Dec 14, 2022

radeusgd left a comment

radeusgd Dec 14, 2022

jdunkerley Dec 14, 2022

radeusgd Dec 14, 2022

radeusgd Dec 14, 2022

jdunkerley Dec 14, 2022

radeusgd Dec 14, 2022

radeusgd Dec 14, 2022

radeusgd Dec 14, 2022

radeusgd Dec 14, 2022

jdunkerley Dec 14, 2022

radeusgd Dec 14, 2022

jdunkerley Dec 14, 2022

hubertp Dec 14, 2022

radeusgd Dec 14, 2022

JSON Improvements, small Table stuff, Statistic in Enso not Java and few other minor bits. #3964

JSON Improvements, small Table stuff, Statistic in Enso not Java and few other minor bits. #3964

Conversation

jdunkerley commented Dec 8, 2022 • edited Loading

Pull Request Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeusgd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdunkerley commented Dec 8, 2022 •

edited

Loading