[jira] [Commented] (TINKERPOP-2888) DefaultTraversal's applyStrategies performance decrease
[
https://issues.apache.org/jira/browse/TINKERPOP-2888?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17713481#comment-17713481
]
Clément de Groc commented on TINKERPOP-2888:
I wouldn't consider our traversals heavily nested and we're only using
{{ReadOnlyStrategy}} at the moment I think (though JanusGraph has [a number of
default
strategies|https://github.com/JanusGraph/janusgraph/blob/v0.6.3/janusgraph-core/src/main/java/org/janusgraph/graphdb/database/StandardJanusGraph.java#L131-L142]
applied automatically).
> DefaultTraversal's applyStrategies performance decrease
> ---
>
> Key: TINKERPOP-2888
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2888
> Project: TinkerPop
> Issue Type: Bug
> Components: server
>Affects Versions: 3.6.1, 3.5.4
>Reporter: Clément de Groc
>Priority: Major
>
> We have recently upgraded from JanusGraph {{0.6.2}} (TinkerPop {{{}3.5.3{}}})
> to JanusGraph {{0.6.3}} (TinkerPop {{{}3.5.5{}}}) and have observed an
> increase in all latency metrics reported by Gremlin Server.
> Using a profiler we have seen that more time is spent in
> {{TraversalHelper.applyTraversalRecursively}} calling itself recursively
> multiple times.
> Then we've narrowed down the issue to [this
> change|https://github.com/apache/tinkerpop/commit/1a548e808922a0eae23b586fe4d6567238989299]
> and resolved the latency problem by reverting the change in
> {{{}DefaultTraversal.java{}}}.
> TLDR; as far as I understand, it looks like, in our case at least, this
> [other round of recursion does have a significant performance
> impact|https://github.com/apache/tinkerpop/pull/1699#issuecomment-1170430130].
> As I'm not too familiar with that part of TinkerPop code, it's unclear to me
> if that's because of our usage of JanusGraph/TinkerPop (feel free to advise).
> But in any case, I thought that was worth reporting in case others hit the
> same issue.
> Happy to share/investigate more if needed.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Created] (TINKERPOP-2926) Gremlin-Java > An UnsupportedOperationException occurs on calling next() after a merge step with the option step modulator if the element does not exist
Andrew Kirk created TINKERPOP-2926:
--
Summary: Gremlin-Java > An UnsupportedOperationException occurs on
calling next() after a merge step with the option step modulator if the element
does not exist
Key: TINKERPOP-2926
URL: https://issues.apache.org/jira/browse/TINKERPOP-2926
Project: TinkerPop
Issue Type: Bug
Components: driver
Affects Versions: 3.6.2
Reporter: Andrew Kirk
Attachments: MergeTestApp.java
Using Gremlin-Java, when the option step modulator is used in combination with
a merge step, an `UnsupportedOperationException` is thrown upon calling
`next()` if the specified element does not already exist.
Using an example from the docs, the following construct works fine in the
console if the element does not already exist:
{code:groovy}
gremlin> g.mergeV([(T.id):300]).
option(Merge.onCreate,[(T.label):'Dog', name:'Toby', age:10]).
option(Merge.onMatch,[age:11])
{code}
But, if we try to do the same thing in Java, we'll get an exception:
{code:java}
g.mergeV(Map.of(T.id, 300))
.option(
Merge.onCreate,
Map.of(
T.label, "Dog",
"name", "Toby",
"age", 10
)
)
.option(
Merge.onMatch,
Map.of("age", 11)
)
.next();
{code}
Exception:
{noformat}
Exception in thread "main" java.lang.UnsupportedOperationException
at
java.base/java.util.ImmutableCollections.uoe(ImmutableCollections.java:142)
at
java.base/java.util.ImmutableCollections$AbstractImmutableMap.putAll(ImmutableCollections.java:1073)
at
org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep.onCreateMap(MergeVertexStep.java:205)
at
org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep.flatMap(MergeVertexStep.java:168)
at
org.apache.tinkerpop.gremlin.process.traversal.step.map.FlatMapStep.processNextStart(FlatMapStep.java:49)
at
org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeStep.processNextStart(MergeStep.java:165)
at
org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep.next(AbstractStep.java:135)
at
org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep.next(AbstractStep.java:40)
at
org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversal.next(DefaultTraversal.java:249)
at
io.integralla.tinkerpop.poc.MergeWithOption.main(MergeWithOption.java:44)
{noformat}
If the element is first added (via an add or merge step), the merge with option
works as expected.
A full example is provided in the attached MergeTestApp.java
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Updated] (TINKERPOP-2926) Gremlin-Java > An UnsupportedOperationException occurs on calling next() after a merge step with the option step modulator if the element does not exist
[
https://issues.apache.org/jira/browse/TINKERPOP-2926?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Yang Xia updated TINKERPOP-2926:
Priority: Blocker (was: Major)
> Gremlin-Java > An UnsupportedOperationException occurs on calling next()
> after a merge step with the option step modulator if the element does not
> exist
>
>
> Key: TINKERPOP-2926
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2926
> Project: TinkerPop
> Issue Type: Bug
> Components: driver
>Affects Versions: 3.6.2
>Reporter: Andrew Kirk
>Priority: Blocker
> Attachments: MergeTestApp.java
>
>
> Using Gremlin-Java, when the option step modulator is used in combination
> with a merge step, an `UnsupportedOperationException` is thrown upon calling
> `next()` if the specified element does not already exist.
> Using an example from the docs, the following construct works fine in the
> console if the element does not already exist:
> {code:groovy}
> gremlin> g.mergeV([(T.id):300]).
> option(Merge.onCreate,[(T.label):'Dog', name:'Toby', age:10]).
> option(Merge.onMatch,[age:11])
> {code}
> But, if we try to do the same thing in Java, we'll get an exception:
> {code:java}
> g.mergeV(Map.of(T.id, 300))
> .option(
> Merge.onCreate,
> Map.of(
> T.label, "Dog",
> "name", "Toby",
> "age", 10
> )
> )
> .option(
> Merge.onMatch,
> Map.of("age", 11)
> )
> .next();
> {code}
> Exception:
> {noformat}
> Exception in thread "main" java.lang.UnsupportedOperationException
> at
> java.base/java.util.ImmutableCollections.uoe(ImmutableCollections.java:142)
> at
> java.base/java.util.ImmutableCollections$AbstractImmutableMap.putAll(ImmutableCollections.java:1073)
> at
> org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep.onCreateMap(MergeVertexStep.java:205)
> at
> org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep.flatMap(MergeVertexStep.java:168)
> at
> org.apache.tinkerpop.gremlin.process.traversal.step.map.FlatMapStep.processNextStart(FlatMapStep.java:49)
> at
> org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeStep.processNextStart(MergeStep.java:165)
> at
> org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep.next(AbstractStep.java:135)
> at
> org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep.next(AbstractStep.java:40)
> at
> org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversal.next(DefaultTraversal.java:249)
> at
> io.integralla.tinkerpop.poc.MergeWithOption.main(MergeWithOption.java:44)
> {noformat}
> If the element is first added (via an add or merge step), the merge with
> option works as expected.
> A full example is provided in the attached MergeTestApp.java
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (TINKERPOP-2855) Performance degradation in TinkerGraph 3.5.4 and 3.5.5
[
https://issues.apache.org/jira/browse/TINKERPOP-2855?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17713714#comment-17713714
]
ASF GitHub Bot commented on TINKERPOP-2855:
---
spmallette opened a new pull request, #2026:
URL: https://github.com/apache/tinkerpop/pull/2026
https://issues.apache.org/jira/browse/TINKERPOP-2855
https://issues.apache.org/jira/browse/TINKERPOP-2888
Removed some recursion. Added `Traversal.lock()` to offer a formal way to
finalize a traversal. Offers a significant performance improvement. Removed
expectation that Graph and related metadata for a `Traversal` is available in
child traversals.
On TINKERPOP-2855 the script demonstrating the slowdown was running at 9.1
seconds on my system for 3.5.5. With this change it is executing at 1.7
seconds.
VOTE +1
> Performance degradation in TinkerGraph 3.5.4 and 3.5.5
> --
>
> Key: TINKERPOP-2855
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2855
> Project: TinkerPop
> Issue Type: Bug
> Components: tinkergraph
>Affects Versions: 3.5.4, 3.5.5
> Environment: Ubuntu 22.04.1, docker
>Reporter: Gleb Sinyavskiy
>Priority: Critical
> Labels: bug, performance
>
> Hello,
> I few days ago I tried to update gremlin-server in our project from 3.5.3 to
> 3.5.4 and found out that it's test suite got 5 times slower. We use the
> official docker image(tinkerpop/gremlin-server) with some configuration
> changes:
> {code:java}
> gremlin.tinkergraph.vertexIdManager=ANY
> gremlin.tinkergraph.edgeIdManager=ANY {code}
> The app uses user-generated string ids and heavily relies on the upsert
> pattern from the [recipes|https://tinkerpop.apache.org/docs/3.5.4/recipes/].
> Yesterday I made an investigation and narrowed it down to the performance of
> the upsert pattern. I also discovered that the issue is not related to our
> configuration changes and can be reproduced with vanilla image and LONG ids.
> I prepared a [simple script that reproduces the
> issue|https://github.com/zhulik/gremlin-server-performance-issue/] and
> contacted Stephen Mallette on discord. They confirmed the issue exists, but
> only in 3.5.4, 3.6.0 performs as expected. They also wrote a groovy script
> that reproduces the problem:
> {code:groovy}
> g = TinkerGraph.open().traversal()
> batches = (0..<100).collect{ (0..<100) }
> start = System.currentTimeMillis()
> for (batch in batches) {
> b = g
> for (id in batch) {
> b = b.V(id).fold().coalesce(__.unfold(), __.addV("test").property(T.id,
> id))
> }
> b.iterate()
> }
> System.currentTimeMillis() - start
> {code}
> [discord
> message|https://discord.com/channels/838910279550238720/838910279550238723/1064964247823593502]
> Both my and Stephen's scripts perform a few times slower when executed
> against 3.5.4 in compare to 3.5.3 or 3.6.0.
> *Steps to reproduce:*
> Run my or Stephen's script against vanilla tinkerpop/gremlin-server:3.5.4
> docker image
> *Expected result:*
> Script's execution time matches it's time when executed against 3.5.3 or 3.6.0
> *Observed result:*
> The script is 5-7 times slower.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (TINKERPOP-2852) Update Maven plugin for docker-images building for M1 compatibility
[ https://issues.apache.org/jira/browse/TINKERPOP-2852?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17713719#comment-17713719 ] ASF GitHub Bot commented on TINKERPOP-2852: --- kenhuuu commented on PR #2007: URL: https://github.com/apache/tinkerpop/pull/2007#issuecomment-1513657376 VOTE +1 > Update Maven plugin for docker-images building for M1 compatibility > --- > > Key: TINKERPOP-2852 > URL: https://issues.apache.org/jira/browse/TINKERPOP-2852 > Project: TinkerPop > Issue Type: Improvement > Components: build-release >Affects Versions: 3.5.4 >Reporter: Yang Xia >Priority: Critical > > The current Maven plug in we use for the `docker-image` building profile is > no longer maintained > ([https://github.com/spotify/dockerfile-maven)|https://github.com/spotify/dockerfile-maven).], > and it is also not compatible with M1 Macs. > We should consider swapping the plug in for an actively maintained one that > is M1 compatible, for example > [https://github.com/fabric8io/docker-maven-plugin/issues/1257.|https://github.com/fabric8io/docker-maven-plugin/issues/1257] -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (TINKERPOP-2855) Performance degradation in TinkerGraph 3.5.4 and 3.5.5
[ https://issues.apache.org/jira/browse/TINKERPOP-2855?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17713722#comment-17713722 ] ASF GitHub Bot commented on TINKERPOP-2855: --- codecov-commenter commented on PR #2026: URL: https://github.com/apache/tinkerpop/pull/2026#issuecomment-1513660499 ## [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/2026?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report > Merging [#2026](https://codecov.io/gh/apache/tinkerpop/pull/2026?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (18067b2) into [3.5-dev](https://codecov.io/gh/apache/tinkerpop/commit/b77a0166be7198a80dc7f7dd775d3342284daf76?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b77a016) will **decrease** coverage by `0.04%`. > The diff coverage is `82.14%`. ```diff @@ Coverage Diff @@ ## 3.5-dev#2026 +/- ## = - Coverage 69.36% 69.32% -0.04% + Complexity 8962 8956 -6 = Files866 866 Lines 4122741229 +2 Branches5434 5434 = - Hits 2859828583 -15 - Misses 1071410736 +22 + Partials1915 1910 -5 ``` | [Impacted Files](https://codecov.io/gh/apache/tinkerpop/pull/2026?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | | |---|---|---| | [...cess/remote/traversal/AbstractRemoteTraversal.java](https://codecov.io/gh/apache/tinkerpop/pull/2026?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3JlbW90ZS90cmF2ZXJzYWwvQWJzdHJhY3RSZW1vdGVUcmF2ZXJzYWwuamF2YQ==) | `5.26% <0.00%> (-0.30%)` | :arrow_down: | | [...tinkerpop/gremlin/process/traversal/Traversal.java](https://codecov.io/gh/apache/tinkerpop/pull/2026?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9UcmF2ZXJzYWwuamF2YQ==) | `76.74% <ø> (ø)` | | | [...gremlin/process/traversal/util/EmptyTraversal.java](https://codecov.io/gh/apache/tinkerpop/pull/2026?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC91dGlsL0VtcHR5VHJhdmVyc2FsLmphdmE=) | `37.50% <0.00%> (-1.21%)` | :arrow_down: | | [...emlin/process/traversal/util/DefaultTraversal.java](https://codecov.io/gh/apache/tinkerpop/pull/2026?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC91dGlsL0RlZmF1bHRUcmF2ZXJzYWwuamF2YQ==) | `84.61% <84.21%> (+0.12%)` | :arrow_up: | | [...cess/traversal/lambda/AbstractLambdaTraversal.java](https://codecov.io/gh/apache/tinkerpop/pull/2026?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9sYW1iZGEvQWJzdHJhY3RMYW1iZGFUcmF2ZXJzYWwuamF2YQ==) | `59.64% <100.00%> (-13.08%)` | :arrow_down: | | [...aversal/strategy/decoration/PartitionStrategy.java](https://codecov.io/gh/apache/tinkerpop/pull/2026?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9zdHJhdGVneS9kZWNvcmF0aW9uL1BhcnRpdGlvblN0cmF0ZWd5LmphdmE=) | `83.23% <100.00%> (ø)` | | | [...remlin/process/traversal/util/TraversalHelper.java](https://codecov.io/gh/apache/tinkerpop/pull/2026?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm
[jira] [Created] (TINKERPOP-2927) Make all Steps extensible and overridable
Oleksandr Porunov created TINKERPOP-2927: Summary: Make all Steps extensible and overridable Key: TINKERPOP-2927 URL: https://issues.apache.org/jira/browse/TINKERPOP-2927 Project: TinkerPop Issue Type: Improvement Components: driver Affects Versions: 3.6.2 Reporter: Oleksandr Porunov Related issue (fixed): https://issues.apache.org/jira/browse/TINKERPOP-2924 Working on optimization strategies sometimes require replacing steps with extended version of those steps. At this moment not all steps can be extended due to being `final` (like `ProjectStep`, `PropertyKeyStep`, `PropertyValueStep`, `RangeLocalStep`, `SumLocalStep`, and many more). Thus, it requires creating a similar step and duplicate some logic there instead of simply extending a specific step. For those steps which are non-final there are sometimes private fields without any getter methods (for example `private CallbackRegistry callbackRegistry` in `DropStep` is `private`. Thus, the caller needs to use Reflaction API to retrieve it's value). In JanusGraph we replace some steps with the extended version of those steps. For example, we completely overwrite `flatMap` step of `PropertiesStep` which is an anti-pattern, but in the case when it's hard to extend specific logic parts such anti-pattern might be a a good solution I guess. I think it would make sense to let Graph developers to extend any step and has access to it's fields / utility methods. In such case we could do similar with `ProjectStep` and make it query data in parallel (see issue: [https://github.com/JanusGraph/janusgraph/issues/3559] ). I'm also good not doing it in case anyone can suggest other patterns to follow for those optimizations instead of overwriting logic. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (TINKERPOP-2927) Make all Steps extensible and overridable
[ https://issues.apache.org/jira/browse/TINKERPOP-2927?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17713784#comment-17713784 ] ASF GitHub Bot commented on TINKERPOP-2927: --- porunov opened a new pull request, #2027: URL: https://github.com/apache/tinkerpop/pull/2027 In this commit the next changes are done for the Step classes: - Make private fields as protected - Allow to overwride any method by removing `final` keyword - Allow to extend any Step class by removing `final` keyword - Add getter methods for some fields which might be useful for Step information retrieval https://issues.apache.org/jira/browse/TINKERPOP-2927 > Make all Steps extensible and overridable > -- > > Key: TINKERPOP-2927 > URL: https://issues.apache.org/jira/browse/TINKERPOP-2927 > Project: TinkerPop > Issue Type: Improvement > Components: driver >Affects Versions: 3.6.2 >Reporter: Oleksandr Porunov >Priority: Minor > > Related issue (fixed): https://issues.apache.org/jira/browse/TINKERPOP-2924 > > Working on optimization strategies sometimes require replacing steps with > extended version of those steps. At this moment not all steps can be extended > due to being `final` (like `ProjectStep`, `PropertyKeyStep`, > `PropertyValueStep`, `RangeLocalStep`, `SumLocalStep`, and many more). Thus, > it requires creating a similar step and duplicate some logic there instead of > simply extending a specific step. > > For those steps which are non-final there are sometimes private fields > without any getter methods (for example `private CallbackRegistry > callbackRegistry` in `DropStep` is `private`. Thus, the caller needs to use > Reflaction API to retrieve it's value). > > In JanusGraph we replace some steps with the extended version of those steps. > For example, we completely overwrite `flatMap` step of `PropertiesStep` which > is an anti-pattern, but in the case when it's hard to extend specific logic > parts such anti-pattern might be a a good solution I guess. > > I think it would make sense to let Graph developers to extend any step and > has access to it's fields / utility methods. > In such case we could do similar with `ProjectStep` and make it query data in > parallel (see issue: [https://github.com/JanusGraph/janusgraph/issues/3559] ). > > I'm also good not doing it in case anyone can suggest other patterns to > follow for those optimizations instead of overwriting logic. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Assigned] (TINKERPOP-2926) Gremlin-Java > An UnsupportedOperationException occurs on calling next() after a merge step with the option step modulator if the element does not exist
[
https://issues.apache.org/jira/browse/TINKERPOP-2926?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Valentyn Kahamlyk reassigned TINKERPOP-2926:
Assignee: Valentyn Kahamlyk
> Gremlin-Java > An UnsupportedOperationException occurs on calling next()
> after a merge step with the option step modulator if the element does not
> exist
>
>
> Key: TINKERPOP-2926
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2926
> Project: TinkerPop
> Issue Type: Bug
> Components: driver
>Affects Versions: 3.6.2
>Reporter: Andrew Kirk
>Assignee: Valentyn Kahamlyk
>Priority: Blocker
> Attachments: MergeTestApp.java
>
>
> Using Gremlin-Java, when the option step modulator is used in combination
> with a merge step, an `UnsupportedOperationException` is thrown upon calling
> `next()` if the specified element does not already exist.
> Using an example from the docs, the following construct works fine in the
> console if the element does not already exist:
> {code:groovy}
> gremlin> g.mergeV([(T.id):300]).
> option(Merge.onCreate,[(T.label):'Dog', name:'Toby', age:10]).
> option(Merge.onMatch,[age:11])
> {code}
> But, if we try to do the same thing in Java, we'll get an exception:
> {code:java}
> g.mergeV(Map.of(T.id, 300))
> .option(
> Merge.onCreate,
> Map.of(
> T.label, "Dog",
> "name", "Toby",
> "age", 10
> )
> )
> .option(
> Merge.onMatch,
> Map.of("age", 11)
> )
> .next();
> {code}
> Exception:
> {noformat}
> Exception in thread "main" java.lang.UnsupportedOperationException
> at
> java.base/java.util.ImmutableCollections.uoe(ImmutableCollections.java:142)
> at
> java.base/java.util.ImmutableCollections$AbstractImmutableMap.putAll(ImmutableCollections.java:1073)
> at
> org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep.onCreateMap(MergeVertexStep.java:205)
> at
> org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep.flatMap(MergeVertexStep.java:168)
> at
> org.apache.tinkerpop.gremlin.process.traversal.step.map.FlatMapStep.processNextStart(FlatMapStep.java:49)
> at
> org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeStep.processNextStart(MergeStep.java:165)
> at
> org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep.next(AbstractStep.java:135)
> at
> org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep.next(AbstractStep.java:40)
> at
> org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversal.next(DefaultTraversal.java:249)
> at
> io.integralla.tinkerpop.poc.MergeWithOption.main(MergeWithOption.java:44)
> {noformat}
> If the element is first added (via an add or merge step), the merge with
> option works as expected.
> A full example is provided in the attached MergeTestApp.java
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (TINKERPOP-2927) Make all Steps extensible and overridable
[ https://issues.apache.org/jira/browse/TINKERPOP-2927?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17713797#comment-17713797 ] ASF GitHub Bot commented on TINKERPOP-2927: --- Cole-Greer commented on PR #2027: URL: https://github.com/apache/tinkerpop/pull/2027#issuecomment-1513920948 Thanks @porunov for submitting this PR. The changes all look valid to me, but opening all of this up is a big change to TinkerPop and needs further consideration. Could you make a [DISCUSS] thread on the dev maillist to ensure that the community is aligned behind this change? > Make all Steps extensible and overridable > -- > > Key: TINKERPOP-2927 > URL: https://issues.apache.org/jira/browse/TINKERPOP-2927 > Project: TinkerPop > Issue Type: Improvement > Components: driver >Affects Versions: 3.6.2 >Reporter: Oleksandr Porunov >Priority: Minor > > Related issue (fixed): https://issues.apache.org/jira/browse/TINKERPOP-2924 > > Working on optimization strategies sometimes require replacing steps with > extended version of those steps. At this moment not all steps can be extended > due to being `final` (like `ProjectStep`, `PropertyKeyStep`, > `PropertyValueStep`, `RangeLocalStep`, `SumLocalStep`, and many more). Thus, > it requires creating a similar step and duplicate some logic there instead of > simply extending a specific step. > > For those steps which are non-final there are sometimes private fields > without any getter methods (for example `private CallbackRegistry > callbackRegistry` in `DropStep` is `private`. Thus, the caller needs to use > Reflaction API to retrieve it's value). > > In JanusGraph we replace some steps with the extended version of those steps. > For example, we completely overwrite `flatMap` step of `PropertiesStep` which > is an anti-pattern, but in the case when it's hard to extend specific logic > parts such anti-pattern might be a a good solution I guess. > > I think it would make sense to let Graph developers to extend any step and > has access to it's fields / utility methods. > In such case we could do similar with `ProjectStep` and make it query data in > parallel (see issue: [https://github.com/JanusGraph/janusgraph/issues/3559] ). > > I'm also good not doing it in case anyone can suggest other patterns to > follow for those optimizations instead of overwriting logic. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[DISCUSS] Relax strictness of Step classes for better extensibility
Hello everyone, I would like to get some opinion from the community regarding relaxing strictness of Step classes for better extensibility. The problem I encounter while making some optimizations for JanusGraph is that many steps are not quite extensible due to being marked as `final` or simply having important fields as `private` without any `public` getters. Thus, making it complicated to replace original TinkerPop Steps with Graph Provider's optimized Steps. In this discussion I would like to know community opinion on relaxing Step classes strictness with making the following changes: 1. Remove `final` keyword from all Step classes, so that it would be possible to extend / overwrite them. Without this change Graph Providers might need to duplicate some logic during their custom Step implementation (because they won't be able to extend an already existing step and instead will need to re-implement it from scratch). Real example: We want to optimize the `ProjectStep` step to be able to query all included traversals in parallel instead of sequentially ( https://github.com/JanusGraph/janusgraph/issues/3559 ). Current TinkerPop's ProjectStep if `final`. Thus, if we want to replace this step with the optimized version of this step we will need to create our custom step which will have most of the logic duplicated with ProjectStep (most likely the only difference will be in the `map` step). 2. Convert all methods and fields from `private` to `protected` to be able to access them in extended Step versions. In case the extended version overwrites some of the original TinkerPop functionality then the child class may potentially need to access some of the parent's class fields. If those fields are private and don't have any getters then it might be complicated to retrieve that information (it will require using Reflection API). Having fields as `protected` will allow for extended child classes to access parent's fields. Another solution could be to have `protected` getters available for those fields. Real example: We would like to optimize PropertyMap step to pre-fetch elements properties ( https://github.com/JanusGraph/janusgraph/issues/2444 ). For that we have to overwrite the `map` method and re-implement it (could be considered as anti-pattern, but it's complicated to do that otherwise). In the overwritten method we may have some duplicated logic to be re-implemented and for that we will need to access parent fields / methods. This was fixed for PropertyMap here ( https://github.com/apache/tinkerpop/pull/2022 ), but there are many other Steps which have private properties without getters. 3. (optional) Add public getters for fields which may be required to be copied from an original Step into an extension Step. Real example: Again about `PropertyMapStep`. Even though we changed the fields to `protected` (which now allows us to set and read those fields from the child class), we can't easily replace the original Step with the extended version of this Step because we will need to provide all the information we have from the original Step into the new Step. Without having public getters available for those fields we won't be able to retrieve data for protected fields (unless we place the optimization class to the same package). I.e. If we implement a replacement Step for PropertyMap step we will need to access `tokens` and `traversalRing` from the original step, but we won't be able to do that because currently PropertyMap doesn't have public getters for those fields. Thus, it will require us to place a new implementation class to the same package, so that we could access protected fields. Overall, I think relaxing Step classes' strictness can benefit Graph Providers as they will be able to overwrite functionality they need. That said, due to small optimization strategies experience, I might miss some development patterns regarding developing optimizations. Potentially, it could be that replacing original Steps is a bad idea but it's hard for me to find good optimization solutions otherwise. These changes don't look to me as breaking changes because no code changes will be required from current Graph Provides sides, but maybe I missed something. As for now, the JanusGraph community has plans to optimize usage of: PropertyMap, ElementMap, Coalesce, Has, Project steps. Thus it drived this thinking and discussion. Ticket regarding relaxing step classes strictness: https://issues.apache.org/jira/browse/TINKERPOP-2927 PR to relax step classes strictness: https://github.com/apache/tinkerpop/pull/2027 Any feedback is appreciated. Best regards, Oleksandr
[jira] [Commented] (TINKERPOP-2927) Make all Steps extensible and overridable
[ https://issues.apache.org/jira/browse/TINKERPOP-2927?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17713820#comment-17713820 ] ASF GitHub Bot commented on TINKERPOP-2927: --- porunov commented on PR #2027: URL: https://github.com/apache/tinkerpop/pull/2027#issuecomment-1514008623 > Thanks @porunov for submitting this PR. The changes all look valid to me, but opening all of this up is a big change to TinkerPop and needs further consideration. Could you make a [DISCUSS] thread on the dev maillist to ensure that the community is aligned behind this change? Thank you for the feedback. I opened the discussion thread here: https://lists.apache.org/thread/vjbjh29kwjhd5lcmkrhw7rw2ynh9 > Make all Steps extensible and overridable > -- > > Key: TINKERPOP-2927 > URL: https://issues.apache.org/jira/browse/TINKERPOP-2927 > Project: TinkerPop > Issue Type: Improvement > Components: driver >Affects Versions: 3.6.2 >Reporter: Oleksandr Porunov >Priority: Minor > > Related issue (fixed): https://issues.apache.org/jira/browse/TINKERPOP-2924 > > Working on optimization strategies sometimes require replacing steps with > extended version of those steps. At this moment not all steps can be extended > due to being `final` (like `ProjectStep`, `PropertyKeyStep`, > `PropertyValueStep`, `RangeLocalStep`, `SumLocalStep`, and many more). Thus, > it requires creating a similar step and duplicate some logic there instead of > simply extending a specific step. > > For those steps which are non-final there are sometimes private fields > without any getter methods (for example `private CallbackRegistry > callbackRegistry` in `DropStep` is `private`. Thus, the caller needs to use > Reflaction API to retrieve it's value). > > In JanusGraph we replace some steps with the extended version of those steps. > For example, we completely overwrite `flatMap` step of `PropertiesStep` which > is an anti-pattern, but in the case when it's hard to extend specific logic > parts such anti-pattern might be a a good solution I guess. > > I think it would make sense to let Graph developers to extend any step and > has access to it's fields / utility methods. > In such case we could do similar with `ProjectStep` and make it query data in > parallel (see issue: [https://github.com/JanusGraph/janusgraph/issues/3559] ). > > I'm also good not doing it in case anyone can suggest other patterns to > follow for those optimizations instead of overwriting logic. -- This message was sent by Atlassian Jira (v8.20.10#820010)
