Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Some minor fixes #3874

Merged
merged 14 commits into from
Nov 17, 2022
Merged

Some minor fixes #3874

merged 14 commits into from
Nov 17, 2022

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Nov 14, 2022 •
edited
Loading

Pull Request Description

  • Allow Map to store a Nothing key (fixes Vector.distinct with a Nothing).
  • Add column_names method to Table as a shorthand.
  • Return data flow error when comparing with Nothing (not a Panic or a Polyglot exception).
  • Allow milli and micro second for DateTime and Time Of Day

Important Notes

  • Added a load of tests for the various comparison operators to Numbers_Spec.

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.

@jdunkerley jdunkerley force-pushed the wip/jd/vector-distinct-nothing branch 2 times, most recently from 3c9e497 to 8977d56 Compare November 15, 2022 10:35
radeusgd
radeusgd reviewed Nov 15, 2022
View reviewed changes
...e/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/bool/CompareToNode.java
var typeError = Context.get(this).getBuiltins().error().makeTypeError(that, bool, "that");
throw new PanicException(typeError, this);
var typeError = Context.get(this).getBuiltins().error().makeTypeError(bool, that, "that");
return DataflowError.withoutTrace(typeError, this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raising this error without any trace will make it harder to debug.
While I appreciate that in many cases we want to avoid adding traces to Dataflow errors for performance, if we are shifting towards using dataflow errors much more than panics, I think we need to revisit that decision.

Maybe we should have 2 classes of errors? Cheap indicator errors that can happen in hot computations that need to be without trace for performance, and the more expensive 'real errors' that used to most often used to be panics but were shifted to dataflow. For the latter 'class', I'd add traces so we can find out where the error has happened.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Producing stacktrace is expensive. However including getSourceSection() - e.g. location in the source should be cheap - if that's enough. The DataflowError has reference to this - it can use this.getSourceSection() later to construct where the error comes from and that shall be fast.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting point.
Still for the more crucial errors I'd consider a full stacktrace as it can give much more information.
Since getting just the source location is so cheap, maybe we should include this for all dataflow errors? That could be quite helpful for debugging still.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I tested the code the getSourceSection didn't appear to work - I think it would be a good addition to the Dataflow error so suggest we add a task to pick this up later.

JaroslavTulach
JaroslavTulach approved these changes Nov 15, 2022
View reviewed changes
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please verify the effect on benchmarks. I think it should be OK, but one can never be sure. Run it for your branch and compare with develop ones.

Consider keeping the more specialized types in the @Specialize methods, if possible.

.../java/org/enso/interpreter/node/expression/builtin/number/bigInteger/GreaterOrEqualNode.java Outdated Show resolved Hide resolved
...e/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/bool/CompareToNode.java
var typeError = Context.get(this).getBuiltins().error().makeTypeError(that, bool, "that");
throw new PanicException(typeError, this);
var typeError = Context.get(this).getBuiltins().error().makeTypeError(bool, that, "that");
return DataflowError.withoutTrace(typeError, this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Producing stacktrace is expensive. However including getSourceSection() - e.g. location in the source should be cheap - if that's enough. The DataflowError has reference to this - it can use this.getSourceSection() later to construct where the error comes from and that shall be fast.

@jdunkerley jdunkerley force-pushed the wip/jd/vector-distinct-nothing branch from 01e1093 to e3d38ad Compare November 16, 2022 10:31
@jdunkerley
Fix issues with Nothing in string compare.
efbb8e2 
Allow Nothing as a key in a Map.
Add column_names short hand to Table.
Add support for a Map in rename_columns.
@jdunkerley
Test graceful failing of Nothing in Text compare.
8b2b20c 
Adjusted Map to cope with Nothing keys better.
Tests for Nothing key in Map.
@jdunkerley
Make Decimal.compare_to error rather than Panic.
7160d7a
@jdunkerley
Working on making compare_to not Panic but Error
2a613ab
@jdunkerley
Adjust compare_to so they return an Error on a mismatched type.
021856a
@jdunkerley
Arguments the wrong way round... (for a long time!)
9ebed3a
@jdunkerley
Update all the number nodes to return data flow errors.
b9196dc
@jdunkerley
Add a load of tests.
30b03c4
@jdunkerley
Milli and Micro second. Update the tests.
d75e415
@jdunkerley
Fix date time tests.
1964b3d 
Make specialisations return type more specific.
@jdunkerley
Changelog
3dac261
@jdunkerley
Return types on Boolean.compare_to.
77aae57 
Fix Vector.distinct so returns error on mixed types.
Test for Table.rename_columns by Map.
Test for Table.column_names.
@jdunkerley
Fix Array.Sort.
bfb914e 
Add assert to check BigInteger doesn't fit in Long.
@jdunkerley jdunkerley force-pushed the wip/jd/vector-distinct-nothing branch from e3d38ad to bfb914e Compare November 16, 2022 10:36
@jdunkerley jdunkerley marked this pull request as ready for review November 16, 2022 10:37
@hubertp
Cleanup dataflow error propagation
a8f322b 
Rather than going through instance checks, introduced a separate node
that uses specializations for that.
In the process, discovered that ComparatorNode was never used or covered
on our test suite. If it did, we would notice that we were comparing
Atoms with AtomConstructors, meaning that we would always throw a panic
when sort is given a comparator. It made more sense to remove that
specialization.
@hubertp hubertp force-pushed the wip/jd/vector-distinct-nothing branch from 0da8b95 to a8f322b Compare November 16, 2022 21:25
@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Nov 17, 2022
@mergify mergify bot merged commit c868ed5 into develop Nov 17, 2022
@mergify mergify bot deleted the wip/jd/vector-distinct-nothing branch November 17, 2022 07:11
@wdanilo wdanilo mentioned this pull request Feb 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radeusgd radeusgd radeusgd approved these changes

@JaroslavTulach JaroslavTulach JaroslavTulach approved these changes

@hubertp hubertp hubertp approved these changes

@4e6 4e6 Awaiting requested review from 4e6 4e6 is a code owner

Assignees
No one assigned
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jdunkerley @hubertp @radeusgd @JaroslavTulach