You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by BrynCooke <gi...@git.apache.org> on 2016/07/04 09:42:06 UTC
[GitHub] tinkerpop pull request #355: TINKERPOP-1355 Design HasContainer for extensio...
GitHub user BrynCooke opened a pull request:
https://github.com/apache/tinkerpop/pull/355
TINKERPOP-1355 Design HasContainer for extension
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/BrynCooke/incubator-tinkerpop TINKERPOP-1355
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/355.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 #355
----
commit ca8a4f9fb8a9bc1d69aa10870d75076b8fa906b5
Author: Bryn Cooke <br...@gmail.com>
Date: 2016-07-04T08:29:40Z
TINKERPOP-1355 Design HasContainer for extension
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #355: TINKERPOP-1355 Design HasContainer for extensio...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/355
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #355: TINKERPOP-1355 Design HasContainer for extension
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/355
I just implemented my proposed changes. Now we no longer get exceptions, but unexpected results:
```
gremlin> g.V().hasId(within(new ArrayList()))
==>v[1]
==>v[2]
==>v[3]
==>v[4]
==>v[5]
==>v[6]
gremlin> g.V().hasId(without(new ArrayList()))
==>v[1]
==>v[2]
==>v[3]
==>v[4]
==>v[5]
==>v[6]
```
This should probably go into a separate ticket.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #355: TINKERPOP-1355 Design HasContainer for extension
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/355
Clean changes. VOTE +1.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #355: TINKERPOP-1355 Design HasContainer for extension
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/355
VOTE +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #355: TINKERPOP-1355 Design HasContainer for extension
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/355
Yea, lots of issues in this code area. `enforceHomogenousCollectionIfPresent` also has the potential to throw NPE's (if the first element of a non-empty collection is `null`). I will create another ticket for those issues.
For this PR:
VOTE: +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #355: TINKERPOP-1355 Design HasContainer for extension
Posted by BrynCooke <gi...@git.apache.org>.
Github user BrynCooke commented on the issue:
https://github.com/apache/tinkerpop/pull/355
Also the conversion of the collection to an array is dangerous. Who knows what the collection is actually backed by? It should use an iterator and get the first element if it exists.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #355: TINKERPOP-1355 Design HasContainer for extension
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/355
I noticed a flaw while reading through the code. It was not introduced by this PR, but it's probably a good time to get it from the table.
```
gremlin> g = TinkerFactory.createModern().traversal()
==>graphtraversalsource[tinkergraph[vertices:6 edges:6], standard]
gremlin> g.V().hasId(within(new ArrayList()))
0
Display stack trace? [yN] N
gremlin> g.V().hasId(without(new ArrayList()))
0
Display stack trace? [yN]
```
A `HasContainer` with an empty collection will always throw an exception because of [line 58 in HasContainer.java](https://github.com/BrynCooke/incubator-tinkerpop/blob/ca8a4f9fb8a9bc1d69aa10870d75076b8fa906b5/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java#L58), where we simply assume that a collection will always contain at least one element.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---