So perhaps a compromise is best. I will fix Jena so that if no effective operator is provided then no NPE will result. However I will also update the Java doc on OpExt to state that if no effective operator is provided implementations do so at their own risk and it may lead to incorrect/Incomplete query optimisation.
Does that seem reasonable? Rob On 04/08/2016 14:33, "Andy Seaborne" <[email protected]> wrote: On 02/08/16 17:01, Rob Vesse wrote: > Potentially yes > > However this was the only source of errors when I upgraded our code > to the latest builds. I will try and concoct some additional test > cases against our code to see if I can hit any of those other places. > We don’t use join classification in our optimiser so we are unlikely > to ever hit that case. We do actually use the variable finder and > once this case was fixed that did not elicit any further issues. > > There is a general question here about the use of OpExt, if the > assumption is that it simply abstracts some specialism of an algebra > then we will fail that assumption. We use it to add additional > features to the query language Beyond what vanilla ARQ it Is able to > support and so there is no effective operator. Returning (table unit) would be possible. The main thing is as a way to declare the variables and how they are used by the OpExt. The effective Op is not used for execution at all. Andy > > Rob > > On 02/08/2016 16:19, "Andy Seaborne" <[email protected]> wrote: > > Rob, > > Several places in the code access effectiveOp during optimization depend > and on there being an equivalent non-null op (e.g. variable finding and > classification). > > Aren't they vulnerable? > > Andy > > On 02/08/16 11:25, [email protected] wrote: > > Repository: jena > > Updated Branches: > > refs/heads/master 61368dfa0 -> c21db9408 > > > > > > Avoid possible NPE in OpVisitor > > > > The default for visit(OpExt) assumed an effectiveOp() is always > > available which may not be the case and can cause a NPE. Check that the > > effectiveOp() is non-null and only visits in that case. > > > > > > Project: http://git-wip-us.apache.org/repos/asf/jena/repo > > Commit: http://git-wip-us.apache.org/repos/asf/jena/commit/c21db940 > > Tree: http://git-wip-us.apache.org/repos/asf/jena/tree/c21db940 > > Diff: http://git-wip-us.apache.org/repos/asf/jena/diff/c21db940 > > > > Branch: refs/heads/master > > Commit: c21db9408a231fbb98173061d048931a8e71dac4 > > Parents: 61368df > > Author: Rob Vesse <[email protected]> > > Authored: Tue Aug 2 11:24:26 2016 +0100 > > Committer: Rob Vesse <[email protected]> > > Committed: Tue Aug 2 11:24:26 2016 +0100 > > > > ---------------------------------------------------------------------- > > .../src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > ---------------------------------------------------------------------- > > > > > > http://git-wip-us.apache.org/repos/asf/jena/blob/c21db940/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java > > ---------------------------------------------------------------------- > > diff --git a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java > > index f947eca..ef3e952 100644 > > --- a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java > > +++ b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java > > @@ -57,7 +57,9 @@ public interface OpVisitor > > public void visit(OpDisjunction opDisjunction) ; > > > > public default void visit(OpExt opExt) { > > - opExt.effectiveOp().visit(this); > > + Op effective = opExt.effectiveOp(); > > + if (effective != null) > > + effective.visit(this); > > } > > > > // OpModifier > > > > > > > >
