afs commented on code in PR #2152:
URL: https://github.com/apache/jena/pull/2152#discussion_r1441903655
##########
jena-extras/jena-querybuilder/src/main/java/org/apache/jena/arq/querybuilder/rewriters/BuildElementVisitor.java:
##########
@@ -125,8 +125,16 @@ public void visit(ElementGroup el) {
// noting to do
result = el;
} else if (lst.size() == 1) {
Review Comment:
@Claudenw What's the background to make the size=1 case special? (noting
2017 was a some time ago!)
It was introduced around Jena 3.7.0 which aligns with #2150.
I don't think the query builder should be making lst of 1 a special case.
There is no harm leaving groups of one. The parser generates them.
The algebra converter, query pretty printing (and OpAsQuery) all have to
understand the group-of-one case. The algebra optimizer deals with them. The
problem is that it does not deal with non-parser generated structures so well.
`ElementFilter` isn't the only example though the algebra converter copes
with bare occurrences of other elements properly.
It is more valuable if the querybuilder generates ASTs like the parser.
`Query.equals` then applies for the case `querybuilder -> string -> parse->
query` and `querybuilder.build -> query`.
If that then triggers querybuilder test failures, there may be a wider issue
lurking.
(PS it does `WhereHandlerTest.testAddGraph` but `WhereValidator` seems to
have its own idea of equality.
##########
jena-extras/jena-querybuilder/src/test/java/org/apache/jena/arq/querybuilder/UpdateBuilderTest.java:
##########
@@ -491,6 +495,38 @@ public void testInsertAndDeleteWithVariableNode() {
assertEquals(builder.makeNode("foo"), t.getObject());
assertTrue(Var.isVar(t.getPredicate()));
}
+
+ @Test
+ public void testInsertAndDeleteWithGraphClauses() {
+ // Test based upon GH-2150
+ // Simplified slightly as the actual URIs used were irrelevant to the
issue
+ RDFConnection conn = RDFConnection.connect(DatasetFactory.create());
+
+ UpdateBuilder updateBuilder = new UpdateBuilder();
+ updateBuilder.addInsert(g, s, p, o);
+
+ SelectBuilder where = new SelectBuilder();
+ where.addWhere(s, p, o);
+ updateBuilder.addGraph(g, where);
+
+
+ SelectBuilder notExists = new SelectBuilder();
+ notExists.addWhere(s, p, o);
+ SelectBuilder where2 = new SelectBuilder();
+ where2.addFilter(where2.getExprFactory().notexists(notExists));
+ updateBuilder.addGraph(g, where2);
+
+ UpdateRequest updateRequest = updateBuilder.buildRequest();
+
+ conn.begin(ReadWrite.WRITE);
+ try {
+ conn.update(updateRequest);
+ conn.commit();
+ } finally {
+ conn.end();
+ }
+ }
+
Review Comment:
Here is a test that focuses on the root cause: not `notexists`, simpler way
to execute `WhereBuilder` to get a group of one.
not sure where it should go - maybe in SelectBuilderTest?
```suggestion
@Test
public void testQueryFilterInGraphClause() {
// Root cause of GH-2150
DatasetGraph dsg = DatasetGraphZero.create();
Node graph1 = SSE.parseNode(":graph");
SelectBuilder selectBuilder = new SelectBuilder();
WhereBuilder whereBuilder = new WhereBuilder();
whereBuilder.addFilter(whereBuilder.makeExpr("true"));
selectBuilder.addGraph(graph1, whereBuilder);
Query query = selectBuilder.build();
QueryExec.dataset(dsg).query(query).select();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
