Skip to content

Core module java rewrite#341

Merged
Ostrzyciel merged 26 commits intomainfrom
core-module-java-rewrite
Apr 23, 2025
Merged

Core module java rewrite#341
Ostrzyciel merged 26 commits intomainfrom
core-module-java-rewrite

Conversation

@nk2IsHere
Copy link
Collaborator

@nk2IsHere nk2IsHere commented Apr 16, 2025

This PR adds core-java and rdf-protos-java modules intended to replace core and rdf-protos, written in scala to ones written in pure java.

This is a part of preparation for v3.0.0 release.
Link to repo before breaking changes: v2.10.2

Notable API Changes:

  • new package naming scheme starting with eu.neverblink is introduced to all new modules
  • JellyConstants, JellyOptions with UPPERCASE_NAMING for constants replaced Constants and Options
  • JellyConverterFactory and JellyTranscoderFactory introduced to centralise core modules context maintaining.
  • ProtoDecoder chain now depends on ProtoHandler<TNode> as a handler of different statement types decoding.
  • ProtoDecoder now does not specify types of Triples and Quads.
  • ProtoEncoder now does not return Iterable<Row> on ingestion, instead a Collection<RdfStreamRow> appendableRowBuffer must be passed through parameters for the purposes of encoded statements collection.

@nk2IsHere nk2IsHere force-pushed the core-module-java-rewrite branch 2 times, most recently from db99436 to 48c8eef Compare April 21, 2025 17:34
@nk2IsHere nk2IsHere marked this pull request as ready for review April 22, 2025 10:02
@nk2IsHere nk2IsHere force-pushed the core-module-java-rewrite branch from 3a4ef02 to 9cf0c90 Compare April 22, 2025 10:03
@nk2IsHere nk2IsHere force-pushed the core-module-java-rewrite branch from ee447a3 to 5842f05 Compare April 22, 2025 11:07
@nk2IsHere nk2IsHere force-pushed the core-module-java-rewrite branch from 5842f05 to 70e381c Compare April 22, 2025 11:28
Copy link
Member

@Ostrzyciel Ostrzyciel left a comment

Choose a reason for hiding this comment

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

partial review, continuing

* Parameters passed to the Jelly encoder.
* <p>
* New fields may be added in the future, but always with a default value and in a sequential order.
* However, it is still recommended to use named arguments when creating this object.
Copy link
Member

Choose a reason for hiding this comment

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

This is out of date, I think.

I haven't worked with Java records. Can we add new fields in a backwards-compatible manner? Or would we need explicit constructors for that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we need explicit constructors for that

Copy link
Member

Choose a reason for hiding this comment

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

Then I'm for adding explicit constructors. This will be wordy and awful, I know, but otherwise introducing a new parameter to the constructor would be a breaking change (!!!!) requiring us to bump the version to 4.0.0. That's bad.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can use factory methods like you did in scala,
or we can use static Params of(...) in Params (aka "named" constructors) to achieve same effect

Copy link
Member

Choose a reason for hiding this comment

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

Sure, you can go with that.

@nk2IsHere nk2IsHere force-pushed the core-module-java-rewrite branch from b3cc9db to 15e354e Compare April 22, 2025 21:41
@Ostrzyciel Ostrzyciel merged commit 972ec40 into main Apr 23, 2025
6 checks passed
@Ostrzyciel Ostrzyciel deleted the core-module-java-rewrite branch April 23, 2025 10:24
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.

2 participants