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

Port POSet to Java #4103

Merged
merged 6 commits into from
Mar 18, 2024
Merged

Port POSet to Java #4103

merged 6 commits into from
Mar 18, 2024

Conversation

Scott-Guest
Copy link
Contributor

@Scott-Guest Scott-Guest commented Mar 14, 2024

Part of #4030.

Port the POSet class from Scala to Java.

This is mostly a straightforward conversion from Scala functional idioms (map, filter, reduce, etc.) to the corresponding Java Stream methods. To port lazy vals, we also implement a Lazy<T> wrapper which caches the result of a Supplier<T>.

The only remaining reference to Scala here is a single constructor which takes a scala.collection.Set<Tuple2<T, T>> and internally converts it to a java.util.Set<Pair<T, T>>. This can be removed once we more pervasively switch away from Scala collection types everywhere in the codebase.

@Scott-Guest Scott-Guest self-assigned this Mar 14, 2024
@Scott-Guest Scott-Guest force-pushed the java-poset branch 4 times, most recently from 8e6c5b1 to 6079e63 Compare March 14, 2024 23:22
@@ -1,2 +1,2 @@
[Error] Compiler: Had 1 parsing errors.
[Error] Compiler: Illegal circular relation: Exp < ExpList < Exp
[Error] Compiler: Illegal circular relation: ExpList < Exp < ExpList
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The output is unstable because it's based on the iteration order of relations.entrySet(), but I don't see a clean way to address this.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can revisit this in the future if it becomes a worse problem; this code is pretty stable and so long as we aren't doing further big changes here we should be fine!

Copy link
Contributor

Choose a reason for hiding this comment

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

If we care more that there's a circularity, and less what the circularity is, we could sed out the example given in the error message.

@Scott-Guest Scott-Guest requested review from Baltoli and gtrepta March 18, 2024 03:45
@Scott-Guest Scott-Guest marked this pull request as ready for review March 18, 2024 03:45
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pleasantly surprised by how nicely the Java code comes out to be here!

import java.util.function.Supplier;

@FunctionalInterface
public interface SerializableSupplier<T> extends Supplier<T>, Serializable {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does the need for Serializable come in? I presume you've thought this all through, just curious what the technical reason is!

Copy link
Contributor Author

@Scott-Guest Scott-Guest Mar 18, 2024

Choose a reason for hiding this comment

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

The root of the issue is that lambdas and method references are not serializable by default. Instead, they are only serializable when constructed in a context whose target type is Serializable (https://stackoverflow.com/questions/25391656/serialization-of-a-lambda-after-its-creation).

In particular, this means that the Supplier<T> in each Lazy<T> in the POSet is not guaranteed to be serializable, giving errors when trying to serialize the overall POSet. We fix this by instead requiring SerializableSupplier<T>.

As for why the overall POSet needs to be serializable in the first place, this was admittedly error driven development - the original POSet class extends Serializable, and removing it causes tests to fail!

Copy link
Contributor

@Baltoli Baltoli 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; I'm impressed by how readable the Java translation is and how minimal the API surface change is. Nice work!

@rv-jenkins rv-jenkins merged commit 2dbd10c into develop Mar 18, 2024
8 checks passed
@rv-jenkins rv-jenkins deleted the java-poset branch March 18, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants