On 05/08/16 09:45, Rob Vesse wrote:
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?
Yes - just the documentation change would be good.
Do your extensions rely on effectiveOp->null ?
I don't think there are will be any NPEs at the moment though obviously
any custom operators have to do various things to play properly.
[There seems to be 2 "OpVisitorByType", old and new (new is the one in
the walker package)]
Andy
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
> >
>
>
>
>
>
>
