afs commented on code in PR #2822:
URL: https://github.com/apache/jena/pull/2822#discussion_r1832829076
##########
jena-arq/src/test/java/org/apache/jena/sparql/api/TestQueryExecutionCancel.java:
##########
@@ -229,6 +247,111 @@ public void test_cancel_json() {
cancellationTest("JSON {\":a\": \"b\"} WHERE {}",
exec->exec.execJson().get(0));
}
+ /**
+ * Registers the function <urn:cancelSignal> which returns its value if
present.
+ * A RuntimeException is raised if there is no cancel signal in the
execution context.
+ *
+ * Note: Prior to jena-5.3.0 the cancel signal was only set when
configuring a timeout.
+ * However, implementations may also want to react to manual abort via
{@link QueryExec#abort()}.
+ */
+ public static FunctionRegistry
registerCancelSignalFunction(FunctionRegistry fnReg) {
+ fnReg.put("urn:cancelSignal", iri -> new FunctionBase0() {
+ @Override
+ protected NodeValue exec(List<NodeValue> args, FunctionEnv env) {
+ ExecutionContext execCxt = (ExecutionContext)env;
+ AtomicBoolean cancelSignal = execCxt.getCancelSignal();
+ if (cancelSignal == null) {
+ throw new RuntimeException("No cancel signal in execution
context.");
+ }
+ return NodeValue.makeBoolean(cancelSignal.get());
+ }
+
+ @Override
+ public NodeValue exec() {
+ throw new IllegalStateException("Should never be called");
+ }
+ });
+
+ return fnReg;
+ }
+
+ /** Set cancel signal function via {@link QueryExecBuilder#set(Symbol,
Object). */
Review Comment:
```suggestion
/** Set cancel signal function via {@link QueryExecBuilder#set(Symbol,
Object)}. */
```
##########
jena-arq/src/main/java/org/apache/jena/http/sys/ExecUpdateHTTPBuilder.java:
##########
@@ -275,25 +276,25 @@ public Y context(Context context) {
if ( context == null )
return thisBuilder();
ensureContext();
- this.context.setAll(context);
+ this.contextAcc.context(context);
return thisBuilder();
}
public Y set(Symbol symbol, Object value) {
ensureContext();
- this.context.set(symbol, value);
+ this.contextAcc.set(symbol, value);
return thisBuilder();
}
public Y set(Symbol symbol, boolean value) {
ensureContext();
- this.context.set(symbol, value);
+ this.contextAcc.set(symbol, value);
return thisBuilder();
}
private void ensureContext() {
- if ( context == null )
- context = Context.create();
+// if ( context == null )
+// context = Context.create();
Review Comment:
Remove.
Changing to `ContextAccumulator` is an improvement.
##########
jena-arq/src/main/java/org/apache/jena/sparql/modify/UpdateEngineWorker.java:
##########
@@ -537,7 +537,7 @@ protected static Iterator<Binding> evalBindings(Query
query, DatasetGraph dsg, B
}
// Not QueryExecDataset.dataset(...) because of initialBinding.
- QueryExecDatasetBuilder builder =
QueryExecDatasetBuilder.create().dataset(dsg).query(query);
+ QueryExecDatasetBuilder builder =
QueryExecDatasetBuilder.create().dataset(dsg).query(query).context(context);
Review Comment:
See comment on the main PR. This is potentially something that impacts users
and we need to consider migration.
##########
jena-arq/src/test/java/org/apache/jena/sparql/api/TestQueryExecutionCancel.java:
##########
@@ -229,6 +247,111 @@ public void test_cancel_json() {
cancellationTest("JSON {\":a\": \"b\"} WHERE {}",
exec->exec.execJson().get(0));
}
+ /**
+ * Registers the function <urn:cancelSignal> which returns its value if
present.
+ * A RuntimeException is raised if there is no cancel signal in the
execution context.
+ *
+ * Note: Prior to jena-5.3.0 the cancel signal was only set when
configuring a timeout.
+ * However, implementations may also want to react to manual abort via
{@link QueryExec#abort()}.
+ */
+ public static FunctionRegistry
registerCancelSignalFunction(FunctionRegistry fnReg) {
+ fnReg.put("urn:cancelSignal", iri -> new FunctionBase0() {
+ @Override
+ protected NodeValue exec(List<NodeValue> args, FunctionEnv env) {
+ ExecutionContext execCxt = (ExecutionContext)env;
+ AtomicBoolean cancelSignal = execCxt.getCancelSignal();
+ if (cancelSignal == null) {
+ throw new RuntimeException("No cancel signal in execution
context.");
+ }
+ return NodeValue.makeBoolean(cancelSignal.get());
+ }
+
+ @Override
+ public NodeValue exec() {
+ throw new IllegalStateException("Should never be called");
+ }
+ });
+
+ return fnReg;
+ }
+
+ /** Set cancel signal function via {@link QueryExecBuilder#set(Symbol,
Object). */
+ @Test
+ public void test_cancel_signal_query_1() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ FunctionRegistry fnReg = registerCancelSignalFunction(new
FunctionRegistry());
+ try (QueryExec qe = QueryExec.dataset(dsg).query("SELECT
(<urn:cancelSignal>() AS ?foobar) { }")
+ .set(ARQConstants.registryFunctions, fnReg)
+ .build()) {
+ Assert.assertEquals(1,
ResultSetFormatter.consume(ResultSet.adapt(qe.select())));
+ }
+ }
+
+ /** Set cancel signal function via {@link
QueryExecBuilder#context(Context). */
Review Comment:
Fix for Javadoc warnings:
```suggestion
/** Set cancel signal function via {@link
QueryExecBuilder#context(Context)}. */
```
also import `QueryExecBuilder`.
##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/QueryExecDataset.java:
##########
@@ -457,6 +457,23 @@ private void startQueryIteratorActual() {
execInit();
+ // JENA-2821 - Unconditionally provide a cancel signal because manual
abort via QueryExec.abort()
+ // may be triggered any time, even if no timeouts were
configured.
+ // Prior to this issue, the cancel signal was only
provided when timeouts were configured.
+
+ // The following note is older:
+ // JENA-2141 - the timeout can go off while building the query
iterator structure.
+ // In this case, use a signal passed through the context.
+ // We don't know if getPlan().iterator() does a lot of work or not
+ // (ideally it shouldn't start executing the query but in some
sub-systems
+ // it might be necessary)
+ //
+ // This applies to the time to first result because to get the first
result, the
+ // queryIterator must have been built. So it does not apply for the
second
+ // stage of N,-1 or N,M.
+ context.set(ARQConstants.symCancelQuery, cancelSignal);
+
+
Review Comment:
Remove one empty line.
##########
jena-arq/src/test/java/org/apache/jena/sparql/api/TestQueryExecutionCancel.java:
##########
@@ -229,6 +247,111 @@ public void test_cancel_json() {
cancellationTest("JSON {\":a\": \"b\"} WHERE {}",
exec->exec.execJson().get(0));
}
+ /**
+ * Registers the function <urn:cancelSignal> which returns its value if
present.
+ * A RuntimeException is raised if there is no cancel signal in the
execution context.
+ *
+ * Note: Prior to jena-5.3.0 the cancel signal was only set when
configuring a timeout.
+ * However, implementations may also want to react to manual abort via
{@link QueryExec#abort()}.
+ */
+ public static FunctionRegistry
registerCancelSignalFunction(FunctionRegistry fnReg) {
+ fnReg.put("urn:cancelSignal", iri -> new FunctionBase0() {
+ @Override
+ protected NodeValue exec(List<NodeValue> args, FunctionEnv env) {
+ ExecutionContext execCxt = (ExecutionContext)env;
+ AtomicBoolean cancelSignal = execCxt.getCancelSignal();
+ if (cancelSignal == null) {
+ throw new RuntimeException("No cancel signal in execution
context.");
+ }
+ return NodeValue.makeBoolean(cancelSignal.get());
+ }
+
+ @Override
+ public NodeValue exec() {
+ throw new IllegalStateException("Should never be called");
+ }
+ });
+
+ return fnReg;
+ }
+
+ /** Set cancel signal function via {@link QueryExecBuilder#set(Symbol,
Object). */
+ @Test
+ public void test_cancel_signal_query_1() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ FunctionRegistry fnReg = registerCancelSignalFunction(new
FunctionRegistry());
+ try (QueryExec qe = QueryExec.dataset(dsg).query("SELECT
(<urn:cancelSignal>() AS ?foobar) { }")
+ .set(ARQConstants.registryFunctions, fnReg)
+ .build()) {
+ Assert.assertEquals(1,
ResultSetFormatter.consume(ResultSet.adapt(qe.select())));
+ }
+ }
+
+ /** Set cancel signal function via {@link
QueryExecBuilder#context(Context). */
+ @Test
+ public void test_cancel_signal_query_2() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ Context cxt = ARQ.getContext().copy();
+ FunctionRegistry fnReg = registerCancelSignalFunction(new
FunctionRegistry());
+ FunctionRegistry.set(cxt, fnReg);
+ try (QueryExec qe = QueryExec.dataset(dsg).query("SELECT
(<urn:cancelSignal>() AS ?foobar) { }").context(cxt).build()) {
+ Assert.assertEquals(1,
ResultSetFormatter.consume(ResultSet.adapt(qe.select())));
+ }
+ }
+
+ /** Set cancel signal function via {@link QueryExec#getContext()}. */
+ @Test
+ public void test_cancel_signal_query_3() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ try (QueryExec qe = QueryExec.dataset(dsg).query("SELECT
(<urn:cancelSignal>() AS ?foobar) { }").build()) {
+ FunctionRegistry fnReg = registerCancelSignalFunction(new
FunctionRegistry());
+ FunctionRegistry.set(qe.getContext(), fnReg);
+ ResultSetFormatter.consume(ResultSet.adapt(qe.select()));
+ }
+ }
+
+ /** Set cancel signal function via {@link UpdateExecBuilder#set(Symbol,
Object)}. */
+ @Test
+ public void test_cancel_signal_update_1() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ FunctionRegistry fnReg = registerCancelSignalFunction(new
FunctionRegistry());
+ UpdateExec ue = UpdateExec.dataset(dsg)
+ .update("INSERT { <s> <p> ?o } WHERE {
BIND(<urn:cancelSignal>() AS ?o) }")
+ .set(ARQConstants.registryFunctions, fnReg)
+ .build();
+ ue.execute();
+ Assert.assertEquals(1, Iter.count(dsg.find()));
+ }
+
+ /** Set cancel signal function via {@link
UpdateExecBuilder#context(Context). */
Review Comment:
```suggestion
/** Set cancel signal function via {@link
UpdateExecBuilder#context(Context)}. */
```
##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/QueryExecDataset.java:
##########
@@ -457,6 +457,23 @@ private void startQueryIteratorActual() {
execInit();
+ // JENA-2821 - Unconditionally provide a cancel signal because manual
abort via QueryExec.abort()
+ // may be triggered any time, even if no timeouts were
configured.
+ // Prior to this issue, the cancel signal was only
provided when timeouts were configured.
+
+ // The following note is older:
+ // JENA-2141 - the timeout can go off while building the query
iterator structure.
+ // In this case, use a signal passed through the context.
+ // We don't know if getPlan().iterator() does a lot of work or not
+ // (ideally it shouldn't start executing the query but in some
sub-systems
+ // it might be necessary)
+ //
+ // This applies to the time to first result because to get the first
result, the
+ // queryIterator must have been built. So it does not apply for the
second
+ // stage of N,-1 or N,M.
Review Comment:
Remove.
##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/http/UpdateExecHTTP.java:
##########
@@ -79,10 +79,10 @@ public static UpdateExecHTTPBuilder service(String
endpointURL) {
this.sendMode = sendMode;
}
-// @Override
-// public Context getContext() {
-// return null;
-// }
+ @Override
+ public Context getContext() {
+ return context;
+ }
//
Review Comment:
Since this class is changing, please remove commented out
`getDatasetGraph()`.
##########
jena-arq/src/test/java/org/apache/jena/sparql/api/TestQueryExecutionCancel.java:
##########
@@ -229,6 +247,111 @@ public void test_cancel_json() {
cancellationTest("JSON {\":a\": \"b\"} WHERE {}",
exec->exec.execJson().get(0));
}
+ /**
+ * Registers the function <urn:cancelSignal> which returns its value if
present.
+ * A RuntimeException is raised if there is no cancel signal in the
execution context.
+ *
+ * Note: Prior to jena-5.3.0 the cancel signal was only set when
configuring a timeout.
+ * However, implementations may also want to react to manual abort via
{@link QueryExec#abort()}.
Review Comment:
I'm not a fan of "historical" documentation in javadoc. That just sticks
around for a very long time. The metric I like is "will it be useful in 5 years
time?". I don't think that passes that test.
I suggest either removing this part or putting in a non-javadoc comment.
##########
jena-arq/src/test/java/org/apache/jena/sparql/api/TestQueryExecutionCancel.java:
##########
@@ -229,6 +247,111 @@ public void test_cancel_json() {
cancellationTest("JSON {\":a\": \"b\"} WHERE {}",
exec->exec.execJson().get(0));
}
+ /**
+ * Registers the function <urn:cancelSignal> which returns its value if
present.
+ * A RuntimeException is raised if there is no cancel signal in the
execution context.
+ *
+ * Note: Prior to jena-5.3.0 the cancel signal was only set when
configuring a timeout.
+ * However, implementations may also want to react to manual abort via
{@link QueryExec#abort()}.
+ */
+ public static FunctionRegistry
registerCancelSignalFunction(FunctionRegistry fnReg) {
+ fnReg.put("urn:cancelSignal", iri -> new FunctionBase0() {
+ @Override
+ protected NodeValue exec(List<NodeValue> args, FunctionEnv env) {
+ ExecutionContext execCxt = (ExecutionContext)env;
+ AtomicBoolean cancelSignal = execCxt.getCancelSignal();
+ if (cancelSignal == null) {
+ throw new RuntimeException("No cancel signal in execution
context.");
+ }
+ return NodeValue.makeBoolean(cancelSignal.get());
+ }
+
+ @Override
+ public NodeValue exec() {
+ throw new IllegalStateException("Should never be called");
+ }
+ });
+
+ return fnReg;
+ }
+
+ /** Set cancel signal function via {@link QueryExecBuilder#set(Symbol,
Object). */
+ @Test
+ public void test_cancel_signal_query_1() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ FunctionRegistry fnReg = registerCancelSignalFunction(new
FunctionRegistry());
+ try (QueryExec qe = QueryExec.dataset(dsg).query("SELECT
(<urn:cancelSignal>() AS ?foobar) { }")
+ .set(ARQConstants.registryFunctions, fnReg)
+ .build()) {
+ Assert.assertEquals(1,
ResultSetFormatter.consume(ResultSet.adapt(qe.select())));
+ }
+ }
+
+ /** Set cancel signal function via {@link
QueryExecBuilder#context(Context). */
+ @Test
+ public void test_cancel_signal_query_2() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ Context cxt = ARQ.getContext().copy();
+ FunctionRegistry fnReg = registerCancelSignalFunction(new
FunctionRegistry());
+ FunctionRegistry.set(cxt, fnReg);
+ try (QueryExec qe = QueryExec.dataset(dsg).query("SELECT
(<urn:cancelSignal>() AS ?foobar) { }").context(cxt).build()) {
+ Assert.assertEquals(1,
ResultSetFormatter.consume(ResultSet.adapt(qe.select())));
+ }
+ }
+
+ /** Set cancel signal function via {@link QueryExec#getContext()}. */
+ @Test
+ public void test_cancel_signal_query_3() {
+ DatasetGraph dsg = DatasetGraphFactory.create();
+ try (QueryExec qe = QueryExec.dataset(dsg).query("SELECT
(<urn:cancelSignal>() AS ?foobar) { }").build()) {
+ FunctionRegistry fnReg = registerCancelSignalFunction(new
FunctionRegistry());
+ FunctionRegistry.set(qe.getContext(), fnReg);
+ ResultSetFormatter.consume(ResultSet.adapt(qe.select()));
+ }
+ }
+
+ /** Set cancel signal function via {@link UpdateExecBuilder#set(Symbol,
Object)}. */
+ @Test
+ public void test_cancel_signal_update_1() {
Review Comment:
I think these should go in new `TestUpdateExecutionCancel`.
##########
jena-arq/src/main/java/org/apache/jena/http/sys/ExecUpdateHTTPBuilder.java:
##########
@@ -275,25 +276,25 @@ public Y context(Context context) {
if ( context == null )
return thisBuilder();
ensureContext();
- this.context.setAll(context);
+ this.contextAcc.context(context);
return thisBuilder();
}
public Y set(Symbol symbol, Object value) {
ensureContext();
- this.context.set(symbol, value);
+ this.contextAcc.set(symbol, value);
return thisBuilder();
}
public Y set(Symbol symbol, boolean value) {
ensureContext();
- this.context.set(symbol, value);
+ this.contextAcc.set(symbol, value);
return thisBuilder();
}
private void ensureContext() {
Review Comment:
Yes, good idea to remove it. "Later" rarely happens.
##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/UpdateExecDatasetBuilder.java:
##########
@@ -43,7 +45,11 @@ public class UpdateExecDatasetBuilder implements
UpdateExecBuilder {
private DatasetGraph dataset = null;
private Query query = null;
- private Context context = null;
+ // private Context context = null;
+ private ContextAccumulator contextAcc =
+ ContextAccumulator.newBuilder(()->ARQ.getContext(),
()->Context.fromDataset(dataset));
+
+
Review Comment:
Remove one empty line.
##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/QueryExecDataset.java:
##########
@@ -457,6 +457,23 @@ private void startQueryIteratorActual() {
execInit();
+ // JENA-2821 - Unconditionally provide a cancel signal because manual
abort via QueryExec.abort()
+ // may be triggered any time, even if no timeouts were
configured.
+ // Prior to this issue, the cancel signal was only
provided when timeouts were configured.
+
+ // The following note is older:
+ // JENA-2141 - the timeout can go off while building the query
iterator structure.
+ // In this case, use a signal passed through the context.
+ // We don't know if getPlan().iterator() does a lot of work or not
+ // (ideally it shouldn't start executing the query but in some
sub-systems
+ // it might be necessary)
+ //
+ // This applies to the time to first result because to get the first
result, the
+ // queryIterator must have been built. So it does not apply for the
second
+ // stage of N,-1 or N,M.
+ context.set(ARQConstants.symCancelQuery, cancelSignal);
+
+
Review Comment:
This is, of course, the one line fix to the original issue!
--
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]
