afs commented on code in PR #2413:
URL: https://github.com/apache/jena/pull/2413#discussion_r1567764379
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/main/VarFinder.java:
##########
@@ -329,12 +336,43 @@ public void visit(OpExtend opExtend) {
private void processAssignVarExprList(VarExprList varExprList) {
varExprList.forEachVarExpr((v,e)-> {
- defines.add(v) ; // Expression may eval to error -> unset?
- if ( e != null )
+ if ( e != null ) {
+ boolean isLikelyToBeDefined = isLikelyToBeDefined(e);
+ if (isLikelyToBeDefined) {
+ defines.add(v) ; // Expression may eval to error ->
unset?
+ }
ExprVars.nonOpVarsMentioned(assignMentions, e);
+ }
}) ;
}
+ /**
+ * Heuristic to check whether the expression's evaluation result is
likely
+ * to always be defined (i.e. never null / type error).
+ * Does not detect runtime type errors, such as when
+ * expr is (?x < 1) and ?x is assigned a string.
+ */
+ private boolean isLikelyToBeDefined(Expr expr) {
+ boolean result;
+ if (expr.isVariable()) {
+ result = defines.contains(expr.asVar());
+ } else if (expr.isConstant() || expr instanceof E_Bound) {
+ result = true;
+ } else if (expr.isFunction()) {
+ ExprFunction fn = expr.getFunction();
+ List<Expr> args = fn.getArgs();
+ if (fn instanceof E_Coalesce) {
+ result = args.stream().anyMatch(this::isLikelyToBeDefined);
+ } else {
+ result = args.stream().allMatch(this::isLikelyToBeDefined);
Review Comment:
```suggestion
result = Iter.anyMatch(args.iterator(),
this::isLikelyToBeDefined);
```
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/main/VarFinder.java:
##########
@@ -351,9 +389,26 @@ public void visit(OpProject opProject) {
assignMentions.addAll(subUsage.assignMentions);
}
+
@Override
public void visit(OpTable opTable) {
- defines.addAll(opTable.getTable().getVars());
+ Table table = opTable.getTable();
+ Set<Var> definedVars = new HashSet<>(table.getVars());
+ List<Var> optVars = new ArrayList<>();
+ Iterator<Binding> rowIt = table.rows();
+ while (rowIt.hasNext()) {
+ Binding row = rowIt.next();
+ Iterator<Var> varIt = definedVars.iterator();
+ while (varIt.hasNext()) {
+ Var var = varIt.next();
+ if (!row.contains(var)) {
+ varIt.remove();
Review Comment:
This method took me a long time to work out! especially `varIt.remove()`.
But being forced into old style code seems necessary.
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/main/VarFinder.java:
##########
@@ -329,12 +336,43 @@ public void visit(OpExtend opExtend) {
private void processAssignVarExprList(VarExprList varExprList) {
varExprList.forEachVarExpr((v,e)-> {
- defines.add(v) ; // Expression may eval to error -> unset?
- if ( e != null )
+ if ( e != null ) {
+ boolean isLikelyToBeDefined = isLikelyToBeDefined(e);
+ if (isLikelyToBeDefined) {
+ defines.add(v) ; // Expression may eval to error ->
unset?
+ }
ExprVars.nonOpVarsMentioned(assignMentions, e);
+ }
}) ;
}
+ /**
+ * Heuristic to check whether the expression's evaluation result is
likely
+ * to always be defined (i.e. never null / type error).
+ * Does not detect runtime type errors, such as when
+ * expr is (?x < 1) and ?x is assigned a string.
+ */
+ private boolean isLikelyToBeDefined(Expr expr) {
+ boolean result;
+ if (expr.isVariable()) {
+ result = defines.contains(expr.asVar());
+ } else if (expr.isConstant() || expr instanceof E_Bound) {
+ result = true;
+ } else if (expr.isFunction()) {
+ ExprFunction fn = expr.getFunction();
+ List<Expr> args = fn.getArgs();
+ if (fn instanceof E_Coalesce) {
+ result = args.stream().anyMatch(this::isLikelyToBeDefined);
Review Comment:
Minor tweak: Streams involve quite a few objects.
```suggestion
result = Iter.anyMatch(args.iteator(),
this::isLikelyToBeDefined);
```
It's lighter weight than `stream().anyMatch` and args is likely small.
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/main/VarFinder.java:
##########
@@ -329,12 +336,43 @@ public void visit(OpExtend opExtend) {
private void processAssignVarExprList(VarExprList varExprList) {
varExprList.forEachVarExpr((v,e)-> {
- defines.add(v) ; // Expression may eval to error -> unset?
- if ( e != null )
+ if ( e != null ) {
+ boolean isLikelyToBeDefined = isLikelyToBeDefined(e);
+ if (isLikelyToBeDefined) {
+ defines.add(v) ; // Expression may eval to error ->
unset?
Review Comment:
Yes - that seems likely.
Not putting it in is quite safe.
Doing so and tests all pass.
##########
jena-arq/src/test/java/org/apache/jena/sparql/core/AbstractTestDynamicDataset.java:
##########
@@ -204,7 +204,17 @@ public void dynamic_dft_7() {
"{ GRAPH <urn:x-arq:DefaultGraph> { ?s <uri:p> ?o . FILTER (
?o IN ( 1, 2) ) } }",
2, dataset);
}
-
+
+ @Test
Review Comment:
It does feel odd. Testing at this level (whole query) is mainly in the
scripted tests.
Please do start a new `Test*` such as `TestQueryExec`.
##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/main/VarFinder.java:
##########
@@ -351,9 +389,26 @@ public void visit(OpProject opProject) {
assignMentions.addAll(subUsage.assignMentions);
}
+
@Override
public void visit(OpTable opTable) {
- defines.addAll(opTable.getTable().getVars());
+ Table table = opTable.getTable();
+ Set<Var> definedVars = new HashSet<>(table.getVars());
+ List<Var> optVars = new ArrayList<>();
+ Iterator<Binding> rowIt = table.rows();
+ while (rowIt.hasNext()) {
+ Binding row = rowIt.next();
+ Iterator<Var> varIt = definedVars.iterator();
+ while (varIt.hasNext()) {
+ Var var = varIt.next();
+ if (!row.contains(var)) {
+ varIt.remove();
+ optVars.add(var);
+ }
+ }
+ }
+ defines.addAll(definedVars);
+ optDefines.addAll(optVars);
Review Comment:
No problem.
--
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: pr-unsubscr...@jena.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@jena.apache.org
For additional commands, e-mail: pr-h...@jena.apache.org
