Skip to content

TINKERPOP-2235 Expand semantics of null #1214

Merged
spmallette merged 23 commits intomasterfrom
TINKERPOP-2235
Dec 3, 2019
Merged

TINKERPOP-2235 Expand semantics of null #1214
spmallette merged 23 commits intomasterfrom
TINKERPOP-2235

Conversation

@spmallette
Copy link
Contributor

@spmallette spmallette commented Nov 6, 2019

https://issues.apache.org/jira/browse/TINKERPOP-2235

This change makes null in traversals behave more consistently and expands its meaning to be a valid value that is not filtered away automatically by the traversal. Please see the upgrade docs for more information on the change as it provides a fair bit of detail.

This is a breaking change heading to 3.5.0 as traversal semantics sorta shift around a bit. It does also enable the possibility of other interesting changes as described in TINKERPOP-2312.

I've tested this manually in the console a fair bit and I'm actually surprised at how well it works without a heavy body of change (despite the size of the PR which is heavily weighted to tests and getting GLVs compliant).

All tests pass with docker/build.sh -t -n -i

@dkuppitz found some bad things during testing. pulling back my vote until they can be resolved. needs more testing....

@spmallette
Copy link
Contributor Author

@dkuppitz - this is good now:

gremlin> g.inject(10,20,null,20,10,10).groupCount("x").dedup().as("y").project("a","b").by().by(__.select("x").select(__.select("y")))
==>[a:10,b:3]
==>[a:20,b:2]
==>[a:null,b:1]

not sure if there are more issues like this or if i even "fixed" it well. hopefully you can find more time to test soon.

@jorgebay i swear i spent almost as much time solving this problem as i did trying to get the c# tests to pass. i had to to this:

https://github.com/apache/tinkerpop/pull/1214/files?file-filters%5B%5D=.cs#diff-c3d769035c1f3cbd767f57d3c9e556c7R40-R41

if you know the fix to that one, please let me know, otherwise it's staying like that for the time being.

@jorgebay
Copy link
Contributor

if you know the fix to that one

Gherkin support in the C# side tries to map text to method calls, with the added difficulty that generics are strict in C# (as opposed to java). IMO It's ok to leave it like this: the C# traversal is OK, is just hard for the test translator to come up with it from text.

@dkuppitz
Copy link
Contributor

VOTE +1

spmallette and others added 23 commits December 3, 2019 11:34
In previous versions null as a value was not allowed as a Traverser in a Traversal. It was part of a filter in DefaultTraversal that would automatically remove it from the stream. While this was a design choice for Gremlin to use null that way, it removed a bit of functionality from Gremlin that could have use at times. As it so happened, the use of null as a filter really didn't need to work that way as we already had a secondary filter which was even better for representing an EmptyTraverser - a bulk of zero or less.
Fix C# gherkin test harness for method with params and generic type parameters.
Add null as token.
The problem that reared its head here is in JavaTranslator (and likely GroovyTranslator) as well where the overloads of Gremlin steps become ambiguous when null is used. Left a big blob comment in JavaTranslator where I came up with a possibly reasonable solution for right now. Added a Graph Feature for null support in property values. Allowed TinkerGraph to support it by default but added a configuration to turn that support off. Neo4j does not support nulls so that feature is disabled for it.
If the graph supports null as a value then it will store it as such. If it does not then setting a property to null is the same as doing a property drop().
Can't rely on null to determine if the traverser is "empty" because null could be the value selected from the Map, Path, etc.
Can't use null for the currentObject - has to be some kind of special indicator. Still not convinced that this is the best way to make this work, but so much of our infrastructure is predicated on null having some special meaning rather than having some property that describes its state.
ScalarMapStep extends and replaces MapStep as the "easy" way to implement a new MapStep. ScalarMapStep works nicely when you don't have to fuss with null logic and is still appropriate for use in those cases where a step will not produce a null value (e.g. addV()). For all other cases, you basically have to extend MapStep directly and implement processNextStart() by hand.
Pulled back all functionality that grabs scope values to Scoping. In this way we have just one place for this logic. Also decided to use exceptions to deal with keys not being found. Not thinking the performance costs of the exception will matter here much as we won't be generating exceptions repeatedly for purpose of flow control - we really just generate them when there is actual error in the traversal syntax and the traversal is headed for exception anyway. if we find that this is a mistaken understanding we can go the way of FastNotSuchElementException and deal with things that way. The alternative would have been to deal with some kind of new return object for the Scoping methods that could indicate whether the key was present or not, but that approach seemed awkward in the various ways I considered it.
Made addV(null) and property(id, null) work more consistently.
Should now throw the same nice error for String and Traversal overloads.
@spmallette spmallette merged commit 05ebdc8 into master Dec 3, 2019
@spmallette spmallette deleted the TINKERPOP-2235 branch December 3, 2019 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants