I like using the with() approach over a strategy, as I think it will be
more user-friendly and discoverable for customers.
I think my main comments are:
1) Let's keep it simple to start with and just allow "All" or "Tokens".
Custom will lead us down a rabbit hole of how far we want to go with allow
different options per label etc. We can release with a simple v1 and
gather feedback from users on how/if they need more granularity.
Meanwhile, customers can still get this behavior through the use of the
current steps such as valueMap()
2) I think we should leverage the same tokens as we do with valueMap() to
help show that they are connected.
3) "detach" is not a word that most people would associate with returning
properties. I suggest something like "materializeProperties"
Combining these together we get the following as the proposed syntax:
g.with(‘materializeProperties’, T.tokens).V()
g.with(‘materializeProperties’, T.all).V()
As far as the default goes, no matter which way we choose, someone will not
be happy. If we choose to default to "tokens", then we are in the same
behavior as currently, which benefits existing applications while making
new applications unhappy. If we choose to default to "all", then this may
be a breaking change for applications but will smooth the on ramp for new
customers as the current behavior is unexpected. Between these two, I
would vote for going with the breaking behavior to simplify the new
developer experience. While it may break some users, I have not often seen
users returning the elements, as it is usually a combination of just an id
or label explicitly or those tokens with all or a subset of properties.
Another advantage to defaulting to returning all the materialized values is
that this will benefit many visualization tools that need to manually
request this information today to render correctly.
Thoughts?
Dave
On Thu, Dec 8, 2022 at 7:41 AM Stephen Mallette <spmalle...@gmail.com>
wrote:
> i was trying to summarize the decisions we're trying to make on this issue
> and it occurred to me that the problems we're facing here raise some
> fundamental questions about the original direction i'd proposed. i long ago
> chose a strategy approach to doing this, mostly because we have some
> history of doing that with HaltedTraverserStrategy and
> ReferenceElementStrategy, but I can't help wondering now if that pattern
> should be followed. It's troubling me that DetachStrategy,
> ReferenceElementStrategy , etc. really don't have any relevance to embedded
> use cases (aside from the very specific case of OLAP with
> HaltedTraverserStrategy). They are really just mechanisms to help control
> serialization costs and as such are more request/response oriented as
> opposed to traversal oriented. If we treated it that way maybe some of
> these hard decisions would go away.
>
> I was thinking that maybe we could add “detachment” to RequestOptions and
> use the same syntax already established for DetachStrategy in there. Hooked
> up properly this would allow something like:
>
> g.with(‘detach’, Detach.custom("name", "age")).V()
>
> On the server, we’d then just have to extract that Detach from the
> RequestMessage and apply it on serialization as needed.
>
> I see several advantages with this revised design:
>
> 1. No more worries about the extra step in profile() - which is even a
> problem now in ReferenceElementStrategy
> 2. This works as easily for scripts as it does for bytecode
> 3. We don’t mix this capability up with embedded use cases, so there’s no
> confusion about when/why you use this.
>
> Going further with the above Gremlin syntax, maybe the argument to with()
> should be like:
>
> g.with(‘detach’, "ALL").V()
> g.with(‘detach’, "NONE").V()
> g.with(‘detach’, ["name", "age"]).V() // a list implies CUSTOM
>
> The advantage here is that we don’t have to change the grammar in
> gremlin-language and the serializers don’t need to change at all. We could
> retain the Detach.custom("name", "age") or similar syntax for Java/Groovy
> and come up with similar sugar for the other languages.
>
> I think that still leaves the question of what the default should be on the
> server for 3.7.0 - perhaps we come back to that once there is feedback on
> this design revision.
>
>
>
> On Tue, Nov 22, 2022 at 7:06 AM Stephen Mallette <spmalle...@gmail.com>
> wrote:
>
> > > My proposal is to always have some detachment strategy in server
> > configuration and add `DetachedStrategy` to allow users the flexibility
> to
> > customize the result.
> >
> > I think default detachment on the server should remain "reference" so if
> > you can do that with your new DetachStrategy then that is preferred with
> > the idea that HaltedTraverserStrategy is no longer useful and can be
> > deprecated.
> >
> > > How to handle more than one DetachedStrategy?
> >
> > I agree that they shouldn't stack - DetachedStrategy should not be used
> > for some sort of security use case. A client-side one should replace the
> > server-side one.
> >
> > > Another question is whether or not to show `DetachElementStep` in the
> > response for profiling requests like `g.V().profile()`.
> >
> > This is the hardest question and goes back to the first because you will
> > be using DetachStrategy by default on the server side and, as a result,
> > when the user does their g.V().profile() without using DetachStrategy
> they
> > will see your DetachElementStep even though they didn't add one on the
> > client side. Right now we hide the detachment behind
> > HaltedTraverserStrategy (for bytecode based requests) and in the default
> > configuration of ReferenceElementStrategy in the startup script. The
> latter
> > makes the former a bit redundant actually.
> >
> > It seems that for script based requests we see ReferenceElementStep now
> in
> > the profile():
> >
> > gremlin> :> g.V().profile()
> > ==>Traversal Metrics
> > Step Count
> > Traversers Time (ms) % Dur
> >
> >
> =============================================================================================================
> > TinkerGraphStep(vertex,[]) 6
> > 6 0.623 37.21
> > ReferenceElementStep 6
> > 6 1.052 62.79
> > >TOTAL -
> > - 1.676 -
> >
> > I don't particularly like that but it might be helpful to see that the
> > detachment is explicitly happening by way of a step.This line of thinking
> > has me wondering if we should remove the default detachment all together
> > from Gremlin Server configuration:
> >
> > 1. Most folks should not be relying on ReferenceElementStrategy to
> protect
> > them from fat elements, because we've spent years pushing them toward
> > valueMap(), project(), etc. If they suddenly started getting properties
> on
> > elements I sense that wouldn't affect many people negatively.
> > 2. With ease of use and intuitive behavior in mind, new users will expect
> > properties to be on their elements.
> > 3. Removing default detachment would make it so that the
> DetachElementStep
> > only appears when the user explicitly sets it themselves. There would be
> no
> > surprise that it is there in the profile().
> > 4. The notion of default detachment would then fall to the serializers
> > which would just include all properties in a DetachedElement.
> >
> > If TinkerPop took this route then we'd have to be sure to warn users of
> > (1) possible increased serialization times if they were relying on
> > elements having no properties and (2) the fact that if they wrote their
> > code to explicitly use a ReferenceVertex they will need to add
> "reference"
> > detachment or generalize their code to the Element interfaces. Maybe some
> > other stuff too?
> >
> >
> >
> >
> >
> >
> >
> > On Fri, Nov 18, 2022 at 5:30 PM Valentyn Kahamlyk
> > <valent...@bitquilltech.com.invalid> wrote:
> >
> >> Hi everyone,
> >>
> >> I'm working on properties on elements (
> >> https://issues.apache.org/jira/browse/TINKERPOP-2824).
> >> I found HaltedTraverserStrategy` in `TraverserIterator` that does about
> >> the
> >> same, converting Elements to detached or references. Tests use
> reference
> >> Elements, and detached is default for production. GLV's possibly can
> >> override strategy to get different results.
> >> My implementation of `DetachedStrategy` is to add the last step to the
> >> traversal which modifies the result, so, technically g.V() becomes
> >> g.V().[DetachStep].
> >> Also default server configuration adds ReferenceElementStrategy
> >>
> >>
> https://github.com/apache/tinkerpop/blob/master/gremlin-server/scripts/empty-sample.groovy#L45
> >> to each traversal.
> >>
> >> 1. So I end up with 3 ways to cut properties and I'm not sure how to
> >> handle
> >> it. What options would you suggest?
> >> My proposal is to always have some detachment strategy in server
> >> configuration and add `DetachedStrategy` to allow users the flexibility
> to
> >> customize the result. `HaltedTraverserStrategy` can be marked deprecated
> >> as
> >> it handles only bytecode traversal, but not used for scripts.
> >> Another side of the problem is the need to detach custom vertex
> >> implementations like `TinkerVertex`.
> >>
> >> 2. How to handle more than one `DetachedStrategy`? For example one can
> be
> >> set in Gremlin Server configuration and other in user traversal.
> >> Options:
> >> a) apply all. Each successive DetachStrategy becomes more restrictive.
> >> b) apply only last one. Use case: there are default server
> configurations
> >> to return only some properties and the user can override it for each
> >> request.
> >> I think both options make sense, but I prefer the second.
> >>
> >> 3. Another question is whether or not to show `DetachElementStep` in the
> >> response for profiling requests like `g.V().profile()`. Users may be
> >> confused at the appearance of a step that they did not add, but on the
> >> other hand it will be an explanation why there are no properties in the
> >> response.
> >> Is it better to hide `DetachElementStep` or vice versa?
> >>
> >> Thanks, Valentyn
> >>
> >
>
