Removed AggregateLocalStep, aggregate(Scope, String), and store()
#3233
Conversation
|
There's mention of
|
...apache/tinkerpop/gremlin/process/traversal/strategy/optimization/EarlyLimitStrategyTest.java
Show resolved
Hide resolved
| {__.aggregate("a").by("name"), | ||
| __.aggregate("a").by(new ValueTraversal<>("name", coalesce(nameValueTraversal, nullTraversal).asAdmin())), | ||
| Collections.emptySet()}, | ||
| {__.aggregate(Scope.local,"a").by("name"), |
There was a problem hiding this comment.
Should these be replaced with local(aggregate())?
There was a problem hiding this comment.
This seems to be a miss. I'll replace.
gremlin-dotnet/test/Gremlin.Net.UnitTest/Process/Traversal/Translator/GroovyTranslatorTests.cs
Show resolved
Hide resolved
.../main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AggregateTest.java
Show resolved
Hide resolved
| ==== Removal of Scopes in aggregate() Step | ||
| Scores are not removed from the `aggregate()` step. The `AggregateStep` is now globally scoped by default. |
There was a problem hiding this comment.
Can you add reasoning why this was done, for example what challenges were caused when it was allowed? Can reference the proposal?
| See: link:https://issues.apache.org/jira/browse/TINKERPOP-3017[TINKERPOP-3017] | ||
| ==== Removal of Scopes in aggregate() Step |
There was a problem hiding this comment.
"Scopes" - plural. how about "aggregate with Scope Removed"?
| ==== Removal of Scopes in aggregate() Step | ||
| The scope parameter has been removed from the `aggregate()` step. The `AggregateStep` is now globally scoped by default. |
There was a problem hiding this comment.
This concepts behind store and aggregate have been around since 3.0 inception. Might be nice to include a little more explanation here as to why this change was deemed necessary.
| g.V().local(aggregate('x')).limit(1).cap('x') | ||
| ---- | ||
| It is important to note that `EarlyLimitStrategy` introduced in 3.3.5 alters the behavior of `aggregate(local)`. |
There was a problem hiding this comment.
hmm, that's a nice side-effect of removal. something to call out in Upgrade Docs to help flesh those out more?
Just a check but have you confirmed that all these examples do in fact return the same results as the ones with aggregate(local)?
There was a problem hiding this comment.
I've manually checked that the examples in the docs run, but I can do a doc build to make sure.
| g.V().hasLabel('person').sideEffect(System.out.&println) <1> | ||
| g.V().sideEffect(outE().count().aggregate(local,"o")). | ||
| sideEffect(inE().count().aggregate(local,"i")).cap("o","i") <2> | ||
| g.V().sideEffect(outE().count().local(aggregate("o"))). |
There was a problem hiding this comment.
Nit: It might be nice to pull all of these docs changes (aside from the semantics docs I suppose) back into the 3.7 deprecation PR. The existing examples in 3.7-dev will still run, however it is preferable to point users towards the preferred local(aggregate()) usage instead of the deprecated aggregate(local)
|
VOTE +1 |
| g.V().where(without("x")).as("a"). | ||
| outE().as("e").inV().as("b"). | ||
| filter(bothE().where(neq("e")).otherV().where(eq("a"))).aggregate(local, "x"). | ||
| filter(bothE().where(neq("e")).otherV().where(eq("a"))).local(aggregate("x")). |
There was a problem hiding this comment.
i think i've asked this elsewhere, but again, have we tested these examples manually to be sure our results are the same just by adding the local wrapping?
| the `mean()` to store in a `List` called "c". Note that `aggregate()` (short form for `aggregate(global)`) was used | ||
| here instead of `aggregate(local)`, as the former is an eager collection of the elements in the stream | ||
| (`aggregate(local)` is lazy) and will force the traversal to be iterated up to that point before moving forward. | ||
| the `mean()` to store in a `List` called "c". Note that `aggregate()` with `local()` was used |
There was a problem hiding this comment.
i think some of this content is in the wrong place.
we deprecated in 3.7.x, so we want folks to start using the new form there. if we only deprecate and don't present the new pattern along with it, how will they understand the new form as they face deprecation? i think the 3.7.x docs should reflect the changes in the reference docs, recipes, etc. in that version, UNLESS, we've changed something in the semantics that only allow these changes in 3.8.0 (in which case there really isn't a deprecation path and maybe the #3234 doesn't make sense? thoughts?
xiazcy
force-pushed
the
3.8-aggregate-local
branch
2 times, most recently
from
084b66b to
5b5a977
Compare
…)` in favor of using `local(aggregate(String))` for lazy aggregation. Updated relevant docs and added additional feature tests.
xiazcy
force-pushed
the
3.8-aggregate-local
branch
from
52afb86 to
144ffe4
Compare
|
VOTE +1 |
4 participants
As follow-up to the Lazy vs Eager evaluation proposal, removed
Scopefromaggregate('x'), which removesAggregateLocalStep, as well as the deprecated equivalent stepstore().AggregateGlobalStepis nowAggregateStepwith default globally scoped (eager aggregation). To achieve lazy aggregation, use the existing steplocal(), which is designed for flow control.Updated relevant docs and added additional tests to ensure
local(aggregate('x'))is semantically equivalent to(aggregate(local, 'x')).