GH-405 add alt bag seq utility functions #2254

reeshabhranjan · 2020-05-21T15:40:48Z

GitHub issue resolved: #405

We have referred RDFCollections.java to implement the consumeContainer() (analogous to consumeCollection()) in the newly created RDFContainers.java, and same for asRDF() method. We are taking the type of collection from the input, and are checking that it should only be one out of RDF.ALT, RDF.BAG and RDF.SEQ.
In the last consumeContainer() method, we have iterated over the supplied values iterable, and added 2 types of predicates for every element:

value rdfs:member container
container rdf:_nnn value

In the above example, container is the actual Resource representing the container in RDF.

Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>

…e-rdf4jGH-405-add-alt-bag-seq

Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>

abrokenjester

Very nice, thanks, good solution. The build failed on formatting issues that you will need to fix. Couple of inline comments and questions.

Also: needs a few unit tests in place, and fixing up the Javadoc.

core/model/src/main/java/org/eclipse/rdf4j/model/util/RDFContainers.java

abrokenjester · 2020-05-22T00:16:57Z

core/model/src/main/java/org/eclipse/rdf4j/model/util/RDFContainers.java

+ */
+public class RDFContainers {
+
+	public static <C extends Collection<Statement>> C asRDF(IRI containerType, Iterable<?> values, Resource container, C sink,


Perhaps a nice addition would be to also have conversion methods that pick the container type, e.g asAlt, asSeq, and asBag.

Actually the containerType passed into the asRDF method is for that purpose only. We will use the containerType parameter in further implementations. Should we have those separate methods as well?

Just a comment on the use of “as”. Java has “toString” and that’s made me use “to” rather than “as” in other code.

@hmottestad I think in this case it's modeled off of the naming we use in the RDFCollections class for similar methods. I kind of agree with you that 'to' might have been a better choice ('as' to me suggests a view on an object rather than a new, converted, object), but to stay consistent I'm fine with sticking with this.

@hmottestad actually, coming back to it, I think you are right, and we might as well change this. @reeshabhkumarranjan do you think you could do a fix to change all the as... method names asRDF, asValues to to... method names (toRDF, toValues, etc.)?

@jeenbroekstra Yes I can do this. Just to confirm, I have to do this only in RDFContainers.java, right?

Yes, that's correct, only RDFContainers. We'll fix RDFCollections later, separately.

…, add untested code for consumeValues() method for RDFContainers.java (documentation pending) Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>

… (documentation pending) Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>

reeshabhranjan · 2020-05-23T07:27:44Z

@jeenbroekstra I will format the file as per guidelines in the final commit, before that, please check my replies to your comments above. Also I have added all the necessary methods in the file. I will soon add the documentation.

Signed-off-by: Prince Sachdeva <prince17080@iiitd.ac.in>

…method Signed-off-by: Prince Sachdeva <prince17080@iiitd.ac.in>

abrokenjester

Excellent work, looking really good. A couple of minor comments only.

core/model/src/main/java/org/eclipse/rdf4j/model/util/RDFContainers.java

reeshabhranjan · 2020-05-23T07:29:05Z

core/model/src/main/java/org/eclipse/rdf4j/model/util/RDFContainers.java

+		Pattern annotatedMembershipPredicatePattern = Pattern.compile("^" + vf.createIRI(RDF.NAMESPACE, "_") + "[1-9][0-9]*$");
+
+		extract(containerType, statementSupplier, container, st -> {
+			if (RDFS.MEMBER.equals(st.getPredicate()) ||
+				annotatedMembershipPredicatePattern.matcher(st.getPredicate().toString()).matches()) {
+				consumer.accept(st.getObject());
+			}
+		}, exceptionSupplier, contexts);
+	}


Here I am using regex to find properties of type rdf:_nnn and simple .equals() to check for rdfs:member.

reeshabhranjan · 2020-05-23T07:30:30Z

core/model/src/main/java/org/eclipse/rdf4j/model/util/RDFContainers.java

+			if (statementSupplier.get(container, annotatedMembershipPredicate, null, contexts).equals(Optional.empty())) {
+				break;
+			}


I will loop through the statements of the form ?s rdf:_nnn ?o until it exists.

reeshabhranjan · 2020-05-23T07:31:32Z

core/model/src/main/java/org/eclipse/rdf4j/model/util/RDFContainers.java

+			if (containerType.equals(RDF.ALT)) {
+				if (encountered.contains(statement.getObject())) {
+					throw exceptionSupplier.apply("rdf:alt cannot contain duplicate values").get();
+				}
+				encountered.add(statement.getObject());
+			}


Only for rdf:Alt, I have added this check to make sure that distinct elements are passed. Instead of throwing the exception, should I just skip adding the duplicate elements?

I'd actually leave this check out, completely. It may seem logical to test this, but there is nothing in the RDF specifications that says anything about the members of an rdf:Alt container having to be distinct.

…un mvn impsort:sort Signed-off-by: Prince Sachdeva <prince17080@iiitd.ac.in>

abrokenjester

Thanks for the latest update, looking good. See my previous response regarding the check you added for rdf:Alt containers.

Also, finally: can you add a few unit tests? Nothing too difficult, just some tests that create some simple containers from Java collections and vice versa. I think you can look at RDFCollectionsTest for inspiration.

reeshabhranjan · 2020-05-25T08:34:24Z

Thanks for the latest update, looking good. See my previous response regarding the check you added for rdf:Alt containers.

Also, finally: can you add a few unit tests? Nothing too difficult, just some tests that create some simple containers from Java collections and vice versa. I think you can look at RDFCollectionsTest for inspiration.

Sure, we will add the tests soon.

…ava, Remove distinct element check for rdf:alt in RDFContainers.java Signed-off-by: Gaurav Aggarwal <gaurav17288@iiitd.ac.in>

abrokenjester

LGTM. Thanks!

abrokenjester · 2020-05-26T01:39:51Z

One final request: could you give your names as you'd like them mentioned for our release notes? We make it a point to mention who contributed to a release, and I want to make sure I get spelling right and include everyone who contributed. Please add a comment to #2235.

reeshabhranjan added 3 commits May 1, 2020 23:53

eclipse-rdf4j#405 Add file for the new feature

41e694b

Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>

Merge branch 'master' of https://github.com/eclipse/rdf4j into eclips…

ea5c0fe

…e-rdf4jGH-405-add-alt-bag-seq

eclipse-rdf4j#405 Add code for consumeCollection for RDFContainers

51793a2

Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>

barthanssens assigned reeshabhranjan May 21, 2020

abrokenjester requested changes May 22, 2020

View reviewed changes

abrokenjester reviewed May 22, 2020

View reviewed changes

reeshabhranjan added 2 commits May 23, 2020 00:32

eclipse-rdf4j#405 Replace assertion with throwing a runtime exception…

99f47b2

…, add untested code for consumeValues() method for RDFContainers.java (documentation pending) Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>

eclipse-rdf4j#405 Add remaining methods (as present in RDFCollection)…

9369b05

… (documentation pending) Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>

prince17080 added 3 commits May 23, 2020 18:33

eclipse-rdf4j#405 Add documentation

d768b86

Signed-off-by: Prince Sachdeva <prince17080@iiitd.ac.in>

eclipse-rdf4j#405 Format the file

bf47ea4

Signed-off-by: Prince Sachdeva <prince17080@iiitd.ac.in>

eclipse-rdf4j#405 Correct the RDFS.Member triple in consumeContainer …

079ef62

…method Signed-off-by: Prince Sachdeva <prince17080@iiitd.ac.in>

abrokenjester reviewed May 23, 2020

View reviewed changes

reeshabhranjan commented May 24, 2020

View reviewed changes

eclipse-rdf4j#405 Change 'as' to 'to', Change version 3.0 to 3.3.0, R…

d1d3546

…un mvn impsort:sort Signed-off-by: Prince Sachdeva <prince17080@iiitd.ac.in>

abrokenjester reviewed May 25, 2020

View reviewed changes

abrokenjester changed the base branch from master to develop May 25, 2020 01:22

abrokenjester changed the title ~~Gh 405 add alt bag seq~~ GH-405 add alt bag seq May 25, 2020

abrokenjester changed the title ~~GH-405 add alt bag seq~~ GH-405 add alt bag seq utility functions May 25, 2020

eclipse-rdf4j#405 Add tests for RDF Containers in RDFContainersTest.j…

7f5c8a1

…ava, Remove distinct element check for rdf:alt in RDFContainers.java Signed-off-by: Gaurav Aggarwal <gaurav17288@iiitd.ac.in>

abrokenjester approved these changes May 26, 2020

View reviewed changes

abrokenjester merged commit 008e215 into eclipse-rdf4j:develop May 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-405 add alt bag seq utility functions #2254

GH-405 add alt bag seq utility functions #2254

reeshabhranjan commented May 21, 2020 •

edited

Loading

abrokenjester left a comment

abrokenjester May 22, 2020

reeshabhranjan May 22, 2020

hmottestad May 23, 2020

abrokenjester May 23, 2020

abrokenjester May 23, 2020

reeshabhranjan May 24, 2020

abrokenjester May 25, 2020

reeshabhranjan commented May 23, 2020

abrokenjester left a comment

reeshabhranjan May 23, 2020

reeshabhranjan May 23, 2020

reeshabhranjan May 23, 2020

abrokenjester May 25, 2020

abrokenjester left a comment •

edited

Loading

reeshabhranjan commented May 25, 2020

abrokenjester left a comment

abrokenjester commented May 26, 2020

GH-405 add alt bag seq utility functions #2254

GH-405 add alt bag seq utility functions #2254

Conversation

reeshabhranjan commented May 21, 2020 • edited Loading

abrokenjester 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

reeshabhranjan commented May 23, 2020

abrokenjester 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

abrokenjester left a comment • edited Loading

Choose a reason for hiding this comment

reeshabhranjan commented May 25, 2020

abrokenjester left a comment

Choose a reason for hiding this comment

abrokenjester commented May 26, 2020

reeshabhranjan commented May 21, 2020 •

edited

Loading

abrokenjester left a comment •

edited

Loading