Hello purple people eaters,
I did some benchmarking of GroupStep vs. GroupStepV3d0. The general themes:
1. GroupStep is slightly faster than GroupV3d0Step (GOOD).
2. GroupStep uses less memory if there is a reducer at the end of the
valueTraversal (e.g. max(), min(), count(), sum(), etc.). (GOOD)
- This is because every "X" inserts its processes the
traversers thus far and reduces them.
------------------
The first query produces 2 keys but lots of values for each key. (actually 3
keys -- we have a data bug in the GratefulDead graph :|).
g.V().both("followedBy").both("followedBy").group().by("songType").by(count())
g.V().both("followedBy").both("followedBy").groupV3d0().by("songType").by().by(count(local))
Both produce the answer:
{cover=368579, =160968, original=771317}
**** The empty key! is a bunch of songs with a blank string for songType. Crap.
Data bug. ****
Runtime group (ms): 420.1875169
Runtime groupV3d0 (ms): 461.91872409999996
Now, without the reducer -- thus, just a big old fold() at the end.
Runtime group (ms): 422.8069872
Runtime groupV3d0 (ms): 458.80180110000003
------------------
The second query produces 338 keys with each key having less values than the
first.
g.V().both("followedBy").both("followedBy").group().by("name").by(count())
g.V().both("followedBy").both("followedBy").groupV3d0().by("name").by().by(count(local))
Runtime group (ms): 428.6962979
Runtime groupV3d0 (ms): 440.7029265
Now, without the reducer -- thus, just a big old fold() at the end.
Runtime group (ms): 442.3166115
Runtime groupV3d0 (ms): 444.6889403
Thus, I think this is a good move as we don't have to worry about OME with
group() and we are getting slightly faster runtimes. I haven't spent much time
optimizing the code either so I suspect we can get GroupStep even faster!
Enjoy,
Marko.
http://markorodriguez.com
On Oct 7, 2015, at 11:52 AM, Marko Rodriguez <[email protected]> wrote:
> Hi everyone,
>
> I was soooooo close to having group() be backwards compatible. However, I
> found a corner case that makes it break.
>
> If you do something like:
>
> g.V.group.by('name').by(outE.weight).by(max(local))
>
> If you did this, I would know you were using "the old way" as your provided a
> reduce by()-modulator. I would simply make your valueTraversal be
> "outE.weight.fold." Easy peasy. In fact, all the test cases passed.
>
> What about if you don't add a reduceTraversal and you want "the old way?"
>
> gremlin> g.V.group.by('name').by(outE.weight) // old way
> ==>[marko:[0.4,0.5,0.1]
>
> gremlin> g.V.group.by('name').by(outE.weight) // new way
> ==>[marko:0.4]
>
> Now, I was thinking, well we can say that if the valueTraversal doesn't end
> with a reducing barrier step, then we add fold() and tada, they are the same.
> However, what about this situation:
>
>
>
> g.V.group.by('name').by(constant(1))
> old way would return a List[1,1,1,1,1,1,1]
> new way returns 1
> now, if you provided a reducer, I would be like: "Ah! you are using the old
> way, let me make it constant(1).fold()"
> but if you don't provide a reducer, then we don't know if you are using old
> or new version.
>
> http://markorodriguez.com
>
> On Oct 7, 2015, at 7:28 AM, Marko Rodriguez <[email protected]> wrote:
>
>> Hi,
>>
>>> - Don't keep the old GropupStep at all, and have the new GroupStep "concat"
>>> the second and third 'by' traversals if they exist.
>>
>> This is not possible. In fact, I messed up my original examples. They should
>> be:
>>
>> g.V.group.by('name').by(outE.weight).by(max(local)) // old way
>> g.V.group.by('name').by(outE.weight.max()) // new way
>>
>> Notice the semantics of the reduce in the old way. Its looking for a
>> collection, not a stream. You can argue, well introspect and flatten/etc.
>> That is easy for max(), sum(), etc, but what about lambdas? What about
>> complex group().by()s people are using I don't even know about and mess up
>> the reasoning on?
>>
>>> - Leave the GroupStep as-is, and add the mapValues step for whoever wants a
>>> better implementation:
>>
>> mapValues() is already step in use.
>>
>> http://tinkerpop.incubator.apache.org/docs/3.1.0-SNAPSHOT/#mapvalues-step
>>
>>> g.V().group().by(keyTraversal).mapValues(valueTraversal).
>>> No deprecation needed.
>>
>> Can't do it cause of mapValues() and we need to keep modulators consistent,
>> hence by()-modulation. I don't want to introduce a new modulator just for
>> this situation as it makes the language start to look hackish. "Oh
>> mapValues() isn't a step, its a modulator like by() as as() but only works
>> for group() as the second modulator?!" Right now we have as() and by() as
>> our "special" modulating steps.
>>
>> Finally, to your notion of group(traversal). We have group(String) which
>> uses GroupSideEffectStep. Furthermore, all step(traversal) are used
>> exclusively for nesting and not for modulation (e.g. local(), repeat(),
>> etc.). Using a traversal parameter in this way would ruin the consistency of
>> the language.
>>
>> Thank you for your thoughts,
>> Marko.
>>
>> http://markorodriguez.com
>>
>>
>>>
>>> On Wed, 7 Oct 2015 at 14:42 Ran Magen <[email protected]> wrote:
>>>
>>>> My point is that group().by('key').by('value/reduce') seems kind of weird
>>>> anyway. The equivalent sql would be "select 'value/reduce' ... group by
>>>> 'key' ".
>>>>
>>>> A few alternative options:
>>>> g.V().group().by(keyTraversal).mapValues(valueTraversal) //As Marko
>>>> pointed out this doesn't address the old GroupStep deprecation
>>>>
>>>> g.V().group(keyTraversal).mapValues(valueTraversal) // group() is
>>>> @Deprecated
>>>>
>>>> g.V().group(valueTraversal).by(keyTraversal) // group() is @Deprecated
>>>>
>>>> g.V().group(keyTraversal, valueTraversal) // group() is @Deprecated
>>>>
>>>>
>>>> On Wed, 7 Oct 2015 at 01:42 Marko Rodriguez <[email protected]> wrote:
>>>>
>>>>> Uh…. First select() is already taken and by() is always a
>>>>> by()-modulator…… :/ Even with that, we still can't fix the backwards
>>>>> compatibility issue as we can't deprecate out the second by(). How do you
>>>>> say "if the second by() is applied, its Deprecated?" Perhaps Logger.WARN
>>>>> or
>>>>> something.. Dunno, sorta odd.
>>>>>
>>>>> Marko.
>>>>>
>>>>> http://markorodriguez.com
>>>>>
>>>>> On Oct 6, 2015, at 3:26 PM, Ran Magen <[email protected]> wrote:
>>>>>
>>>>>> How about changing the second 'by' to a more indicative name (could
>>>>>> 'select' work?)?
>>>>>>
>>>>>> That way we could improve on the name (which always had me confused),
>>>>> and
>>>>>> leave the second+third 'by's as an indication to use the old
>>>>>> implementation...
>>>>>>
>>>>>> On יום ד׳, 7 באוק׳ 2015 at 0:18 Marko Rodriguez <[email protected]>
>>>>>> wrote:
>>>>>>
>>>>>>> Hello everyone (and the NSA -- Kuppitz did it),
>>>>>>>
>>>>>>> This is regarding this ticket:
>>>>>>> https://issues.apache.org/jira/browse/TINKERPOP3-866
>>>>>>>
>>>>>>> In group_step_2/ branch we have a new implementation of GroupStep.
>>>>> Here is
>>>>>>> an example:
>>>>>>>
>>>>>>> g.V.group.by('name').by(outE.weight).by(max()) // old way
>>>>>>> g.V.group.by('name').by(outE.weight.max()) // new way
>>>>>>>
>>>>>>> What is the difference, there is only two by()-modulators that can be
>>>>>>> passed to group(): keyTraversal and valueTraversal. What happened to
>>>>>>> reduceTraversal? Well, valueTraversal is that. If you want to reduce on
>>>>>>> your values, well, add a reducing barrier step -- e.g. max, min, sum,
>>>>>>> count, fold, mean, etc. In the old way, we would put all your values
>>>>> into a
>>>>>>> Collection<V> and then when the step was complete, we would feed those
>>>>> into
>>>>>>> the reduceTraversal. Now -- we can dynamically reduce on the fly. This
>>>>> will
>>>>>>> greatly improve speed and memory usage as we don't have to create a
>>>>> big old
>>>>>>> Collection<V> and instead, can reduce as new traversers are fed into
>>>>>>> valueTraversal.
>>>>>>>
>>>>>>> So this is great and no would object. What sucks is that this is NOT
>>>>>>> backwards compatible. What Stephen and I came up with (in the ticket)
>>>>> is to
>>>>>>> make:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>> https://github.com/apache/incubator-tinkerpop/blob/group_step_2/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java#L637-L639
>>>>>>>
>>>>>>> This new step groupV3d0() is a deprecated step that is what the "old
>>>>> way"
>>>>>>> was. Note the naming convention that Stephen used for Kryo is now used
>>>>> here
>>>>>>> -- V3d0 (version 3<dot>0). Thus, for users moving forward, they will
>>>>> have
>>>>>>> to do a "replace all" on their queries and rename "group()" to
>>>>>>> "groupV3d0()". Then slowly, over time, convert their traversal to use
>>>>> the
>>>>>>> new group() step and its respective semantics.
>>>>>>>
>>>>>>> Note that in the past we have been able to do backwards compatibility
>>>>> via
>>>>>>> having the new step (e.g. AddVertexStep) simulate the behavior of the
>>>>> old
>>>>>>> step and have respective @Deprecates in there to say that particular
>>>>>>> GraphTraversal methods will go away. However, in this situation, the
>>>>> best
>>>>>>> we can do is groupV3d0().
>>>>>>>
>>>>>>> Anywho. That is that. If people have any better ideas on how to solve
>>>>> this
>>>>>>> backwards compatibility issue, I'm all (deaf) ears.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Marko.
>>>>>>>
>>>>>>> http://markorodriguez.com
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>
>
