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.

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

Enso Date shall be converted to java.time.LocalDate when passed to Java #3374

Merged
merged 6 commits into from
Apr 15, 2022

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Apr 1, 2022

Pull Request Description

This PR provides a unit test, an integration test and code changes to guarantee automatic conversion of java.time.LocalDate to Enso Date atom and back.

Originally (after the first few commits) the following test failed:

sbt:runtime> testOnly *DateTest*
 - should compare dates in java *** FAILED ***
[info]     org.enso.interpreter.test.InterpreterException: class com.oracle.truffle.polyglot.PolyglotMap cannot be cast to class java.time.chrono.ChronoLocalDate (com.oracle.truffle.polyglot.PolyglotMap is in module org.graalvm.truffle of loader 'app'; java.time.chrono.ChronoLocalDate is in module java.base of loader 'bootstrap')
[info]     at java.base/java.time.LocalDate.compareTo(LocalDate.java:139)
[info]     at <enso>.argument<1>(<interactive_source>:3)
[info]     at <enso>.<eval>(Unknown)
[info]     at <enso>.Debug.eval(Unknown)
[info]     at <enso>.Test.main(Test:6)
[info]     at org.graalvm.sdk/org.graalvm.polyglot.Value.execute(Value.java:841)

then this PR seeks to provide a fix. The PR has been enhanced with another test that sends java.time.LocalDate back to Enso and fails with:

 - should check java date has enso methods *** FAILED ***
[info]     org.enso.interpreter.test.InterpreterException: Method `year` of java.time.LocalDate could not be found.
[info]     at <enso>.Test.main(Test:7)
[info]     at org.graalvm.sdk/org.graalvm.polyglot.Value.execute(Value.java:841)

Now all the unit as well as integration tests pass OK.

Important Notes

Checklist

Please include the following checklist in your PR:

  • [ x ] The documentation has been updated if necessary.
  • [ x ] All code conforms to the Scala, Java, and Rust style guides.
  • All code has been tested:
    • [ x ] Unit tests have been written where possible.

@JaroslavTulach JaroslavTulach requested a review from 4e6 as a code owner April 1, 2022 05:16
@CLAassistant
Copy link

CLAassistant commented Apr 1, 2022

CLA assistant check
All committers have signed the CLA.

@jdunkerley
Copy link
Member

Think this would convert any Atom where first field is a LocalDate, which feels wider.

The original problem, I was meaning was coercing Standard.Base.Data.Time.Date to java.time.LocalDate and vice versa when we cross over the polyglot boundary.

I don't believe this fix would do anything when coming back with a LocalDate to Enso.

Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

We need to put more thought into API design here.

@jdunkerley
Copy link
Member

I agree with @kustosz suggestion.

I think we probably need to make Date/DateTime/Time part of the built in space, but it would be also helpful to control how an Enso Atom is converted across the boundary. This clearly needs some API design.

@JaroslavTulach
Copy link
Member Author

I don't believe this fix would do anything when coming back with a LocalDate to Enso.

Right, it didn't do anything. Thanks for pointing that out, James. Since d4a8e31 it is tested.

I think we probably need to make Date/DateTime/Time part of the built in space,

As soon as the engine encounters a foreign object that represents a date, it needs to wrap it/convert it/mimic it by Enso Date. That means either that the Engine knows about the Date atom or its shape or ...

helpful to control how an Enso Atom is converted across the boundary. This clearly needs some API design.

...there is an API between the Engine and the Date library that allows it to enhance foreign object that claim to be isDate().

I guess I try to hack something first and then we can think of proper API to do it nicer.

Copy link
Member Author

@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.

I guess we are ready for another round of review. Thanks in advance for your comments.

@JaroslavTulach JaroslavTulach requested a review from kustosz April 9, 2022 05:31
Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

Very nice. Some nitpicks / discussion points inline. Also please make sure it doesn't make things slower.

JaroslavTulach added a commit to JaroslavTulach/enso that referenced this pull request Apr 14, 2022
@jtulach jtulach force-pushed the jtulach/LocalDate branch from 028335a to eee9bf2 Compare April 14, 2022 08:18
@JaroslavTulach
Copy link
Member Author

Squashing the commits into:

  • the test 413179e
  • the fix eee9bf2
  • additional doc/import polishing commits

@JaroslavTulach JaroslavTulach requested a review from kustosz April 14, 2022 08:20
Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

Looks good, there's just one stylistic choice I disagree with and it seems like code formatting is off in general. Approving, as fixing these shouldn't require another review from me :)

@mwu-tow mwu-tow merged commit ab692b3 into enso-org:develop Apr 15, 2022
mergify bot pushed a commit that referenced this pull request Jul 21, 2022
Significantly improves the polyglot Date support (as introduced by #3374). It enhances the `Date_Spec` to run it in four flavors:
- with Enso Date (as of now)
- with JavaScript Date
- with JavaScript Date wrapped in (JavaScript) array
- with Java LocalDate allocated directly

The code is then improved by necessary modifications to make the `Date_Spec` pass.

# Important Notes
James has requested in [#181755990](https://www.pivotaltracker.com/n/projects/2539304/stories/181755990) - e.g. _Review and improve InMemory Table support for Dates, Times, DateTimes, BigIntegers_ the following program to work:
```
foreign js dateArr = """
return [1, new Date(), 7]

main =
IO.println <| (dateArr.at 1).week_of_year
```
the program works with here in provided changes and prints `27` as of today.

@jdunkerley has provided tests for proper behavior of date in `Table` and `Column`. Those tests are working as of [f16d07e](f16d07e). One just needs to accept `List<Value>` and then query `Value` for `isDate()` when needed.

Last round of changes is related to **exception handling**. 8b686b1 makes sure `makePolyglotError` accepts only polyglot values. Then it wraps plain Java exceptions into `WrapPlainException` with `has_type` method - 60da5e7 - the remaining changes in the PR are only trying to get all tests working in the new setup.

The support for `Time` isn't part of this PR yet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants