[GitHub] jena pull request #394: JENA-1519: OpWalkerVisitor with custom operators

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/jena/pull/394


---

[GitHub] jena pull request #394: JENA-1519: OpWalkerVisitor with custom operators

[afs]

Github user afs commented on a diff in the pull request:

https://github.com/apache/jena/pull/394#discussion_r182114929
  
--- Diff: 
jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpWalker.java ---
@@ -139,6 +139,7 @@ protected void visitN(OpN op) {
 @Override
 protected void visitExt(OpExt op) {
 before(op) ;
+super.visitExt(op);
--- End diff --

There are probably both cases. Its an extension point so it is hard to be 
categorical. Only the effective is walkable unless the OpExt is comprised of 
known Ops in which case it does not need to be an OpExt.

Let's leave it as per the PR - we might have to return to it but without a 
concrete alternative usage, it feels like slipping towards guessing.



---

[GitHub] jena pull request #394: JENA-1519: OpWalkerVisitor with custom operators

[jeremy-coulon]

Github user jeremy-coulon commented on a diff in the pull request:

https://github.com/apache/jena/pull/394#discussion_r181407237
  
--- Diff: 
jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpWalker.java ---
@@ -139,6 +139,7 @@ protected void visitN(OpN op) {
 @Override
 protected void visitExt(OpExt op) {
 before(op) ;
+super.visitExt(op);
--- End diff --

Visiting the real OpExt is not useful for me but I can't say for other 
people.


---

[GitHub] jena pull request #394: JENA-1519: OpWalkerVisitor with custom operators

[afs]

Github user afs commented on a diff in the pull request:

https://github.com/apache/jena/pull/394#discussion_r181393545
  
--- Diff: 
jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpWalker.java ---
@@ -139,6 +139,7 @@ protected void visitN(OpN op) {
 @Override
 protected void visitExt(OpExt op) {
 before(op) ;
+super.visitExt(op);
--- End diff --

Not sure about this. It is visiting both the effective op and the real 
OpExt. Shoudn't it be the visitor deciding that? ie. visiting 
`op.effectiveOp()` is required?


---

[GitHub] jena pull request #394: JENA-1519: OpWalkerVisitor with custom operators

[jeremy-coulon]

GitHub user jeremy-coulon opened a pull request:

https://github.com/apache/jena/pull/394

JENA-1519: OpWalkerVisitor with custom operators



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/jeremy-coulon/jena bug-opext-jira-1519

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/jena/pull/394.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #394


commit 74529be4f7da2ce1a16c5542df073a1911859751
Author: Jeremy Coulon 
Date:   2018-04-10T15:19:29Z

Add UT showing the problem

commit 9c2bb1c183a70fafa1007857823e91aca9a1f18f
Author: Jeremy Coulon 
Date:   2018-04-10T15:20:08Z

Fix OpWalkerVisitor for OpExt




---