[GitHub] tinkerpop pull request #876: TINKERPOP-967 Support nested-repeat() structure...
Github user dkuppitz commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/876#discussion_r202456800 --- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/B_LP_NL_O_P_S_SE_SL_Traverser.java --- @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal.traverser; + +import org.apache.commons.collections.map.ReferenceMap; +import org.apache.tinkerpop.gremlin.process.traversal.Step; +import org.apache.tinkerpop.gremlin.process.traversal.traverser.util.LabelledCounter; + + +import java.util.Iterator; +import java.util.Stack; + +public class B_LP_NL_O_P_S_SE_SL_Traverser extends B_LP_O_P_S_SE_SL_Traverser { + +protected Stack nestedLoops; +protected ReferenceMap loopNames = null; --- End diff -- Hmm, to be realistic, I don't expect more than 2 or 3 named repeat steps in a single traversal, so an index for the names might really not be necessary (IMO). ---
[jira] [Commented] (TINKERPOP-967) Support nested-repeat() structures
[ https://issues.apache.org/jira/browse/TINKERPOP-967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16543630#comment-16543630 ] ASF GitHub Bot commented on TINKERPOP-967: -- Github user dkuppitz commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/876#discussion_r202453831 --- Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStepTest.java --- @@ -39,7 +39,9 @@ __.repeat(out()).times(3), __.repeat(out().as("x")).times(3), __.out().emit().repeat(out()).times(3), -__.repeat(out()).until(hasLabel("x")) +__.repeat(out()).until(hasLabel("x")), +__.repeat("a", __.out()).times(3), --- End diff -- Ah, alright, I totally missed the first entry, all good then. We would have to shift or rotate the `loopName`'s hashCode value by the depth of the repeat traversal. Unfortunately, we don't have this information and I don't see how your `hashCode` implementation above would solve the problem. Anyway, it's an unlikely corner case and I don't think we need to care about it too much, so no big deal if we don't have a solution for now. > Support nested-repeat() structures > -- > > Key: TINKERPOP-967 > URL: https://issues.apache.org/jira/browse/TINKERPOP-967 > Project: TinkerPop > Issue Type: Improvement > Components: process >Affects Versions: 3.1.0-incubating >Reporter: Marko A. Rodriguez >Assignee: Marko A. Rodriguez >Priority: Major > > All the internal plumbing is staged for this to happen, we just haven't gone > all the way. In short, a {{NESTED_LOOP}} traverser has an internal > {{loopStack}} where {{repeat(repeat())}} will have a {{loopStack}} of two. > The {{it.loops()}} checks of the internal repeat will always check the top of > the stack and when its done repeating will delete its counter off the top of > the stack. > [~dkuppitz]'s work on {{LoopStep}} will be backwards compatible. In > {{RepeatStep}} we will support: > {code} > repeat('a',out('knows').repeat('b',out('parent'))) > {code} > and thus, things like {{loops('a')}} as well as {{times('a',2)}}. Note that > naming the loop stack will be a super rare case as most people will just > assume standard nested looping semantics with a push/pop stack. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[GitHub] tinkerpop pull request #876: TINKERPOP-967 Support nested-repeat() structure...
Github user dkuppitz commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/876#discussion_r202453831 --- Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStepTest.java --- @@ -39,7 +39,9 @@ __.repeat(out()).times(3), __.repeat(out().as("x")).times(3), __.out().emit().repeat(out()).times(3), -__.repeat(out()).until(hasLabel("x")) +__.repeat(out()).until(hasLabel("x")), +__.repeat("a", __.out()).times(3), --- End diff -- Ah, alright, I totally missed the first entry, all good then. We would have to shift or rotate the `loopName`'s hashCode value by the depth of the repeat traversal. Unfortunately, we don't have this information and I don't see how your `hashCode` implementation above would solve the problem. Anyway, it's an unlikely corner case and I don't think we need to care about it too much, so no big deal if we don't have a solution for now. ---
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/876 VOTE: +1 ---
[GitHub] tinkerpop pull request #876: TINKERPOP-967 Support nested-repeat() structure...
Github user GCHQResearcher1337 commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/876#discussion_r202452106 --- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/B_LP_NL_O_P_S_SE_SL_Traverser.java --- @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal.traverser; + +import org.apache.commons.collections.map.ReferenceMap; +import org.apache.tinkerpop.gremlin.process.traversal.Step; +import org.apache.tinkerpop.gremlin.process.traversal.traverser.util.LabelledCounter; + + +import java.util.Iterator; +import java.util.Stack; + +public class B_LP_NL_O_P_S_SE_SL_Traverser extends B_LP_O_P_S_SE_SL_Traverser { + +protected Stack nestedLoops; +protected ReferenceMap loopNames = null; --- End diff -- Sure. So the loop stack is a stack of LabelledCounters which store the stepId and the loop counter. As per the second part of https://issues.apache.org/jira/browse/TINKERPOP-967 we want to be able to 'name' a loop counter. I could store this in the LabelledCounter alongside the stepId and the counter, but to get more constant time lookup by name I use a separate Map from loop name -> LabelledCounter. When `resetLoops()` is called the counter is removed from the Stack and also needs to be removed from this lookup Map. Because the map is keyed on 'name' not stepId, I don't know which entry to remove without either: * Checking every entry on the map to see if the LabelledCounter matches the one I'm removing from the stack. * Storing the 'name' in the stack of loop counter A Map with 'Weak values' only holds onto entries while the value (the reference to the LabelledCounter) is valid. When it gets released the entry (the 'name' and the Reference to the LabelledCounter) get removed. This avoids needing a second Map/second index/extra to store lookups from I may have overcomplicated this. I was trying to avoid looping over the loopStack (which I expect to be small) and to keep storage down (which is hard to do) - especially considering the existing code seems to want to shave as many bytes as possible off the looping storage (using a short under the hood when the interface exposes an int): https://github.com/apache/tinkerpop/blob/a80eb84169048ed74c5ad27ebc4d12944fd0136a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/B_O_S_SE_SL_Traverser.java#L34 ---
[GitHub] tinkerpop pull request #876: TINKERPOP-967 Support nested-repeat() structure...
Github user spmallette commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/876#discussion_r202446882 --- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/B_LP_NL_O_P_S_SE_SL_Traverser.java --- @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal.traverser; + +import org.apache.commons.collections.map.ReferenceMap; +import org.apache.tinkerpop.gremlin.process.traversal.Step; +import org.apache.tinkerpop.gremlin.process.traversal.traverser.util.LabelledCounter; + + +import java.util.Iterator; +import java.util.Stack; + +public class B_LP_NL_O_P_S_SE_SL_Traverser extends B_LP_O_P_S_SE_SL_Traverser { + +protected Stack nestedLoops; +protected ReferenceMap loopNames = null; --- End diff -- Could you explain a bit further why a weak reference is desirable here? You mentioned that it helps simplify `resetLoops()` but I'm unfortunately not making the connection as to why even with the phraseology you followed with about not needing "to walk the Map or store the 'loop name' multiple times." ---
[jira] [Commented] (TINKERPOP-967) Support nested-repeat() structures
[ https://issues.apache.org/jira/browse/TINKERPOP-967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16543605#comment-16543605 ] ASF GitHub Bot commented on TINKERPOP-967: -- Github user GCHQResearcher1337 commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/876#discussion_r202445268 --- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/B_LP_NL_O_P_S_SE_SL_Traverser.java --- @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal.traverser; + +import org.apache.commons.collections.map.ReferenceMap; +import org.apache.tinkerpop.gremlin.process.traversal.Step; +import org.apache.tinkerpop.gremlin.process.traversal.traverser.util.LabelledCounter; + + +import java.util.Iterator; +import java.util.Stack; + +public class B_LP_NL_O_P_S_SE_SL_Traverser extends B_LP_O_P_S_SE_SL_Traverser { + +protected Stack nestedLoops; +protected ReferenceMap loopNames = null; --- End diff -- Yep :) magic. I think at garbage collection the entry will get removed, or there will be on access checks of the validity of the reference. I've used something similar before (can't quite remember what) but didn't want to introduce an extra dependency, but luckily I found ReferenceMap in org.apache.commons.collections which was already included and seems to do this. I wasn't quite sure if these references would be safe to 'attaching' as I don't fully understand this, but I think attaching only moves a traverser to a different part of the graph so the References/counters should still be safe. https://github.com/apache/tinkerpop/blob/a80eb84169048ed74c5ad27ebc4d12944fd0136a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/Attachable.java#L62 > Support nested-repeat() structures > -- > > Key: TINKERPOP-967 > URL: https://issues.apache.org/jira/browse/TINKERPOP-967 > Project: TinkerPop > Issue Type: Improvement > Components: process >Affects Versions: 3.1.0-incubating >Reporter: Marko A. Rodriguez >Assignee: Marko A. Rodriguez >Priority: Major > > All the internal plumbing is staged for this to happen, we just haven't gone > all the way. In short, a {{NESTED_LOOP}} traverser has an internal > {{loopStack}} where {{repeat(repeat())}} will have a {{loopStack}} of two. > The {{it.loops()}} checks of the internal repeat will always check the top of > the stack and when its done repeating will delete its counter off the top of > the stack. > [~dkuppitz]'s work on {{LoopStep}} will be backwards compatible. In > {{RepeatStep}} we will support: > {code} > repeat('a',out('knows').repeat('b',out('parent'))) > {code} > and thus, things like {{loops('a')}} as well as {{times('a',2)}}. Note that > naming the loop stack will be a super rare case as most people will just > assume standard nested looping semantics with a push/pop stack. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (TINKERPOP-967) Support nested-repeat() structures
[ https://issues.apache.org/jira/browse/TINKERPOP-967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16543591#comment-16543591 ] ASF GitHub Bot commented on TINKERPOP-967: -- Github user GCHQResearcher1337 commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/876#discussion_r202442057 --- Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStepTest.java --- @@ -39,7 +39,9 @@ __.repeat(out()).times(3), __.repeat(out().as("x")).times(3), __.out().emit().repeat(out()).times(3), -__.repeat(out()).until(hasLabel("x")) +__.repeat(out()).until(hasLabel("x")), +__.repeat("a", __.out()).times(3), --- End diff -- I don't quite understand your first point - `__.repeat(out()).times(3)` is the first entry in the array and I believe this did fail before the `hashCode()` fix. Good point about the collision. Maybe I could use the same method the emit first and until first use? ie: ``` @Override public int hashCode() { int result = super.hashCode() ^ (this.repeatTraversal.hashCode() << 1); result ^= Boolean.hashCode(this.untilFirst); result ^= Boolean.hashCode(this.emitFirst) << 1; if (this.loopName != null) result ^= this.loopName.hashCode(); if (this.untilTraversal != null) result ^= this.untilTraversal.hashCode(); if (this.emitTraversal != null) result ^= this.emitTraversal.hashCode(); return result; } ``` > Support nested-repeat() structures > -- > > Key: TINKERPOP-967 > URL: https://issues.apache.org/jira/browse/TINKERPOP-967 > Project: TinkerPop > Issue Type: Improvement > Components: process >Affects Versions: 3.1.0-incubating >Reporter: Marko A. Rodriguez >Assignee: Marko A. Rodriguez >Priority: Major > > All the internal plumbing is staged for this to happen, we just haven't gone > all the way. In short, a {{NESTED_LOOP}} traverser has an internal > {{loopStack}} where {{repeat(repeat())}} will have a {{loopStack}} of two. > The {{it.loops()}} checks of the internal repeat will always check the top of > the stack and when its done repeating will delete its counter off the top of > the stack. > [~dkuppitz]'s work on {{LoopStep}} will be backwards compatible. In > {{RepeatStep}} we will support: > {code} > repeat('a',out('knows').repeat('b',out('parent'))) > {code} > and thus, things like {{loops('a')}} as well as {{times('a',2)}}. Note that > naming the loop stack will be a super rare case as most people will just > assume standard nested looping semantics with a push/pop stack. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[GitHub] tinkerpop pull request #876: TINKERPOP-967 Support nested-repeat() structure...
Github user GCHQResearcher1337 commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/876#discussion_r202442057 --- Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStepTest.java --- @@ -39,7 +39,9 @@ __.repeat(out()).times(3), __.repeat(out().as("x")).times(3), __.out().emit().repeat(out()).times(3), -__.repeat(out()).until(hasLabel("x")) +__.repeat(out()).until(hasLabel("x")), +__.repeat("a", __.out()).times(3), --- End diff -- I don't quite understand your first point - `__.repeat(out()).times(3)` is the first entry in the array and I believe this did fail before the `hashCode()` fix. Good point about the collision. Maybe I could use the same method the emit first and until first use? ie: ``` @Override public int hashCode() { int result = super.hashCode() ^ (this.repeatTraversal.hashCode() << 1); result ^= Boolean.hashCode(this.untilFirst); result ^= Boolean.hashCode(this.emitFirst) << 1; if (this.loopName != null) result ^= this.loopName.hashCode(); if (this.untilTraversal != null) result ^= this.untilTraversal.hashCode(); if (this.emitTraversal != null) result ^= this.emitTraversal.hashCode(); return result; } ``` ---
[GitHub] tinkerpop pull request #876: TINKERPOP-967 Support nested-repeat() structure...
Github user dkuppitz commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/876#discussion_r202438650 --- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/B_LP_NL_O_P_S_SE_SL_Traverser.java --- @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal.traverser; + +import org.apache.commons.collections.map.ReferenceMap; +import org.apache.tinkerpop.gremlin.process.traversal.Step; +import org.apache.tinkerpop.gremlin.process.traversal.traverser.util.LabelledCounter; + + +import java.util.Iterator; +import java.util.Stack; + +public class B_LP_NL_O_P_S_SE_SL_Traverser extends B_LP_O_P_S_SE_SL_Traverser { + +protected Stack nestedLoops; +protected ReferenceMap loopNames = null; --- End diff -- So, for my own education, does that mean that `ReferenceMap` automagically removes entries from the map, when there's no more reference to the value (e.g. when you pop a labeled counter from the stack)? ---
[jira] [Commented] (TINKERPOP-967) Support nested-repeat() structures
[ https://issues.apache.org/jira/browse/TINKERPOP-967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16543582#comment-16543582 ] ASF GitHub Bot commented on TINKERPOP-967: -- Github user dkuppitz commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/876#discussion_r202438650 --- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/B_LP_NL_O_P_S_SE_SL_Traverser.java --- @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal.traverser; + +import org.apache.commons.collections.map.ReferenceMap; +import org.apache.tinkerpop.gremlin.process.traversal.Step; +import org.apache.tinkerpop.gremlin.process.traversal.traverser.util.LabelledCounter; + + +import java.util.Iterator; +import java.util.Stack; + +public class B_LP_NL_O_P_S_SE_SL_Traverser extends B_LP_O_P_S_SE_SL_Traverser { + +protected Stack nestedLoops; +protected ReferenceMap loopNames = null; --- End diff -- So, for my own education, does that mean that `ReferenceMap` automagically removes entries from the map, when there's no more reference to the value (e.g. when you pop a labeled counter from the stack)? > Support nested-repeat() structures > -- > > Key: TINKERPOP-967 > URL: https://issues.apache.org/jira/browse/TINKERPOP-967 > Project: TinkerPop > Issue Type: Improvement > Components: process >Affects Versions: 3.1.0-incubating >Reporter: Marko A. Rodriguez >Assignee: Marko A. Rodriguez >Priority: Major > > All the internal plumbing is staged for this to happen, we just haven't gone > all the way. In short, a {{NESTED_LOOP}} traverser has an internal > {{loopStack}} where {{repeat(repeat())}} will have a {{loopStack}} of two. > The {{it.loops()}} checks of the internal repeat will always check the top of > the stack and when its done repeating will delete its counter off the top of > the stack. > [~dkuppitz]'s work on {{LoopStep}} will be backwards compatible. In > {{RepeatStep}} we will support: > {code} > repeat('a',out('knows').repeat('b',out('parent'))) > {code} > and thus, things like {{loops('a')}} as well as {{times('a',2)}}. Note that > naming the loop stack will be a super rare case as most people will just > assume standard nested looping semantics with a push/pop stack. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (TINKERPOP-967) Support nested-repeat() structures
[ https://issues.apache.org/jira/browse/TINKERPOP-967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16543575#comment-16543575 ] ASF GitHub Bot commented on TINKERPOP-967: -- Github user dkuppitz commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/876#discussion_r202437245 --- Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStepTest.java --- @@ -39,7 +39,9 @@ __.repeat(out()).times(3), __.repeat(out().as("x")).times(3), __.out().emit().repeat(out()).times(3), -__.repeat(out()).until(hasLabel("x")) +__.repeat(out()).until(hasLabel("x")), +__.repeat("a", __.out()).times(3), --- End diff -- Sorry, perhaps I should have been more clear on what kind of traversals to add here. Can you please also add `__.repeat(out()).times(3)`? Without the changes in `hashCode()` this traversal would have had the same hashCode as `__.repeat("a", out()).times(3)` and thus this test would have failed. I guess these two will still collide: ``` __.repeat("a", __.repeat("b", out())) __.repeat("b", __.repeat("a", out())) ``` ...but I can't think of a solution that doesn't involve any expensive method calls. > Support nested-repeat() structures > -- > > Key: TINKERPOP-967 > URL: https://issues.apache.org/jira/browse/TINKERPOP-967 > Project: TinkerPop > Issue Type: Improvement > Components: process >Affects Versions: 3.1.0-incubating >Reporter: Marko A. Rodriguez >Assignee: Marko A. Rodriguez >Priority: Major > > All the internal plumbing is staged for this to happen, we just haven't gone > all the way. In short, a {{NESTED_LOOP}} traverser has an internal > {{loopStack}} where {{repeat(repeat())}} will have a {{loopStack}} of two. > The {{it.loops()}} checks of the internal repeat will always check the top of > the stack and when its done repeating will delete its counter off the top of > the stack. > [~dkuppitz]'s work on {{LoopStep}} will be backwards compatible. In > {{RepeatStep}} we will support: > {code} > repeat('a',out('knows').repeat('b',out('parent'))) > {code} > and thus, things like {{loops('a')}} as well as {{times('a',2)}}. Note that > naming the loop stack will be a super rare case as most people will just > assume standard nested looping semantics with a push/pop stack. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[GitHub] tinkerpop pull request #876: TINKERPOP-967 Support nested-repeat() structure...
Github user dkuppitz commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/876#discussion_r202437245 --- Diff: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStepTest.java --- @@ -39,7 +39,9 @@ __.repeat(out()).times(3), __.repeat(out().as("x")).times(3), __.out().emit().repeat(out()).times(3), -__.repeat(out()).until(hasLabel("x")) +__.repeat(out()).until(hasLabel("x")), +__.repeat("a", __.out()).times(3), --- End diff -- Sorry, perhaps I should have been more clear on what kind of traversals to add here. Can you please also add `__.repeat(out()).times(3)`? Without the changes in `hashCode()` this traversal would have had the same hashCode as `__.repeat("a", out()).times(3)` and thus this test would have failed. I guess these two will still collide: ``` __.repeat("a", __.repeat("b", out())) __.repeat("b", __.repeat("a", out())) ``` ...but I can't think of a solution that doesn't involve any expensive method calls. ---
[jira] [Commented] (TINKERPOP-967) Support nested-repeat() structures
[ https://issues.apache.org/jira/browse/TINKERPOP-967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16543525#comment-16543525 ] ASF GitHub Bot commented on TINKERPOP-967: -- Github user GCHQResearcher1337 commented on the issue: https://github.com/apache/tinkerpop/pull/876 @dkuppitz Thanks for your comments. I think I've fixed the issues you spotted. > Support nested-repeat() structures > -- > > Key: TINKERPOP-967 > URL: https://issues.apache.org/jira/browse/TINKERPOP-967 > Project: TinkerPop > Issue Type: Improvement > Components: process >Affects Versions: 3.1.0-incubating >Reporter: Marko A. Rodriguez >Assignee: Marko A. Rodriguez >Priority: Major > > All the internal plumbing is staged for this to happen, we just haven't gone > all the way. In short, a {{NESTED_LOOP}} traverser has an internal > {{loopStack}} where {{repeat(repeat())}} will have a {{loopStack}} of two. > The {{it.loops()}} checks of the internal repeat will always check the top of > the stack and when its done repeating will delete its counter off the top of > the stack. > [~dkuppitz]'s work on {{LoopStep}} will be backwards compatible. In > {{RepeatStep}} we will support: > {code} > repeat('a',out('knows').repeat('b',out('parent'))) > {code} > and thus, things like {{loops('a')}} as well as {{times('a',2)}}. Note that > naming the loop stack will be a super rare case as most people will just > assume standard nested looping semantics with a push/pop stack. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[GitHub] tinkerpop issue #876: TINKERPOP-967 Support nested-repeat() structures
Github user GCHQResearcher1337 commented on the issue: https://github.com/apache/tinkerpop/pull/876 @dkuppitz Thanks for your comments. I think I've fixed the issues you spotted. ---
[jira] [Commented] (TINKERPOP-967) Support nested-repeat() structures
[ https://issues.apache.org/jira/browse/TINKERPOP-967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16543459#comment-16543459 ] ASF GitHub Bot commented on TINKERPOP-967: -- Github user GCHQResearcher1337 commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/876#discussion_r202418371 --- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/util/LabelledCounter.java --- @@ -0,0 +1,85 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.tinkerpop.gremlin.process.traversal.traverser.util; + +import org.apache.commons.lang.mutable.MutableShort; + +import java.io.Serializable; + +/** + * Class to track a count associated with a Label + */ +public class LabelledCounter implements Serializable, Cloneable { + +private final String label; +private final MutableShort count = new MutableShort(); --- End diff -- Whoops that my mistake. I'll change it to a primitive short. It's a short to match the loop counters used elsewhere e.g. https://github.com/GCHQResearcher1337/tinkerpop/blob/a80eb84169048ed74c5ad27ebc4d12944fd0136a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/B_O_S_SE_SL_Traverser.java#L34 But I originally used a Pair<> before switching to a class, which prevented me from using primitive types. > Support nested-repeat() structures > -- > > Key: TINKERPOP-967 > URL: https://issues.apache.org/jira/browse/TINKERPOP-967 > Project: TinkerPop > Issue Type: Improvement > Components: process >Affects Versions: 3.1.0-incubating >Reporter: Marko A. Rodriguez >Assignee: Marko A. Rodriguez >Priority: Major > > All the internal plumbing is staged for this to happen, we just haven't gone > all the way. In short, a {{NESTED_LOOP}} traverser has an internal > {{loopStack}} where {{repeat(repeat())}} will have a {{loopStack}} of two. > The {{it.loops()}} checks of the internal repeat will always check the top of > the stack and when its done repeating will delete its counter off the top of > the stack. > [~dkuppitz]'s work on {{LoopStep}} will be backwards compatible. In > {{RepeatStep}} we will support: > {code} > repeat('a',out('knows').repeat('b',out('parent'))) > {code} > and thus, things like {{loops('a')}} as well as {{times('a',2)}}. Note that > naming the loop stack will be a super rare case as most people will just > assume standard nested looping semantics with a push/pop stack. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[GitHub] tinkerpop pull request #876: TINKERPOP-967 Support nested-repeat() structure...
Github user GCHQResearcher1337 commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/876#discussion_r202418371 --- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/util/LabelledCounter.java --- @@ -0,0 +1,85 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.tinkerpop.gremlin.process.traversal.traverser.util; + +import org.apache.commons.lang.mutable.MutableShort; + +import java.io.Serializable; + +/** + * Class to track a count associated with a Label + */ +public class LabelledCounter implements Serializable, Cloneable { + +private final String label; +private final MutableShort count = new MutableShort(); --- End diff -- Whoops that my mistake. I'll change it to a primitive short. It's a short to match the loop counters used elsewhere e.g. https://github.com/GCHQResearcher1337/tinkerpop/blob/a80eb84169048ed74c5ad27ebc4d12944fd0136a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/B_O_S_SE_SL_Traverser.java#L34 But I originally used a Pair<> before switching to a class, which prevented me from using primitive types. ---
[jira] [Commented] (TINKERPOP-967) Support nested-repeat() structures
[ https://issues.apache.org/jira/browse/TINKERPOP-967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16543449#comment-16543449 ] ASF GitHub Bot commented on TINKERPOP-967: -- Github user GCHQResearcher1337 commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/876#discussion_r202415964 --- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/B_LP_NL_O_P_S_SE_SL_Traverser.java --- @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal.traverser; + +import org.apache.commons.collections.map.ReferenceMap; +import org.apache.tinkerpop.gremlin.process.traversal.Step; +import org.apache.tinkerpop.gremlin.process.traversal.traverser.util.LabelledCounter; + + +import java.util.Iterator; +import java.util.Stack; + +public class B_LP_NL_O_P_S_SE_SL_Traverser extends B_LP_O_P_S_SE_SL_Traverser { + +protected Stack nestedLoops; +protected ReferenceMap loopNames = null; --- End diff -- I'm using a map with weak values to simplify the `resetLoops()` call so that I don't need to walk the Map or store the 'loop name' multiple times. > Support nested-repeat() structures > -- > > Key: TINKERPOP-967 > URL: https://issues.apache.org/jira/browse/TINKERPOP-967 > Project: TinkerPop > Issue Type: Improvement > Components: process >Affects Versions: 3.1.0-incubating >Reporter: Marko A. Rodriguez >Assignee: Marko A. Rodriguez >Priority: Major > > All the internal plumbing is staged for this to happen, we just haven't gone > all the way. In short, a {{NESTED_LOOP}} traverser has an internal > {{loopStack}} where {{repeat(repeat())}} will have a {{loopStack}} of two. > The {{it.loops()}} checks of the internal repeat will always check the top of > the stack and when its done repeating will delete its counter off the top of > the stack. > [~dkuppitz]'s work on {{LoopStep}} will be backwards compatible. In > {{RepeatStep}} we will support: > {code} > repeat('a',out('knows').repeat('b',out('parent'))) > {code} > and thus, things like {{loops('a')}} as well as {{times('a',2)}}. Note that > naming the loop stack will be a super rare case as most people will just > assume standard nested looping semantics with a push/pop stack. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
Re: [DISCUSS] Review Process
That looks good, thanks Stephen. --Ted On Thu, Jul 12, 2018 at 5:23 PM Stephen Mallette wrote: > I'm going to quickly summarize this thread so that hopefully by Monday we > have a plan to follow. > > We seem to agree that in the future we will go with the following review > -the-commit (RTC) process: > > 1. Each change to TinkerPop release branches requires 3 +1s from committers > and no -1s OR > 2. A single +1 from a committer and a 1 week review period for objections > (i.e. "cool down period") at which point we will assume a lazy consensus > 3. The single +1 may come from the committer who submitted the PR in the > first place > 4. Committers are trusted with their changes but are expected to request > reviews for complex changes as necessary and not rely strictly on lazy > consensus > 5. commit-the-review (CTR) procedures remain unchanged > > If there are no final comments, I will adjust our dev documentation > accordingly (and then merge a bunch of PRs!) > > Thanks for everyone's participation, > > Stephen > > On Wed, Jul 11, 2018 at 6:50 AM Stephen Mallette > wrote: > > > oops - Pieter, i read your post last night then replied this morning > > thinking I remembered everything you wrote - you actually called for > > different email/jira lists as well.I guess that would be helpful to > > some but not others. For me personally, that would be massively > burdensome > > tbh. > > > > On Wed, Jul 11, 2018 at 6:46 AM Stephen Mallette > > wrote: > > > >> Thanks for everyone's thoughts - some replies, first to Jason: > >> > >> > but I agree that nagging is not a great path forward. > >> > >> Robert Dale has already expressed his sadness that my nags are going > away > >> > >> > It'd be great to have these examples added to the maintainer > >> guidelines. > >> > >> i've said as much before in different places but it's not in the dev > >> docs. of course, depending on how this thread ends the dev docs will > need > >> some changes in this area all around so we'll see about adding such > things. > >> > >> > If the contribution is a major feature or significant change, the > >> expectation is that the committer realizes this and holds it open for 3 > >> votes before committing. > >> > >> So, here's my thinking on "major feature" - if the committer had done > >> things properly the "major feature" would have already had major > >> visibility. There would have been some form of DISCUSS thread ahead of > >> time, plus a JIRA ticket with more information. If it was a "major" > third > >> party submission, a committer would not +1 and walk away but suggest to > the > >> third-party that we need to bring this to the attention of dev in some > way. > >> How about this, perhaps in the DISCUSS/JiRA anyone might call for a > "review > >> of 3" (reserving of course for a -1 veto) in which case the "cooling > >> period" wouldn't apply and we'd need the 3 +1s from committers. > >> > >> As for Pieter: > >> > >> > Perhaps for version 4 the project should be broken up > >> > >> Maybe, though I've come to to really like our giant uber project. Having > >> everything together has so many great benefits and somehow we've > managed to > >> keep the whole thing relatively easy to build and deploy. Breaking up > the > >> project repositories however would not allow you to really hide from the > >> expansiveness we've grown in to. This dev list would still be the > location > >> for all discussion and we'd still likely have a single JIRA instance so > I > >> don't see that providing much relief if you're feeling that way > >> unfortunately. > >> > >> > >> > >> On Tue, Jul 10, 2018 at 4:19 PM pieter gmail > >> wrote: > >> > >>> Hi, > >>> > >>> I feel like the project has become a bit too big and dispersed. A large > >>> portion of the emails, jira or otherwise are irrelevant to my > >>> interest/time/work. > >>> > >>> Perhaps for version 4, TinkerPop could be broken up into more focused > >>> projects with their own jira/email/process management. > >>> > >>> gremlin-language > >>> gremlin-server > >>> js-driver > >>> python-driver > >>> java-driver > >>> .net-driver > >>> reference implementation > >>> ... > >>> > >>> Thanks > >>> Pieter > >>> > >>> > >>> > >>> > >>> Perhaps for version 4 the project should be broken up > >>> > >>> On 10/07/2018 22:01, Jason Plurad wrote: > >>> > Thanks for starting this conversation, Stephen. Lots of interesting > >>> tidbits > >>> > here, and perhaps some we can apply to other OSS projects. > >>> > > >>> >> I'm not sure if committers/PMC members have just not had time to do > >>> > reviews or have not felt comfortable doing them > >>> > > >>> > Probably a combination of both, especially with the GLVs. > >>> > > >>> >> I personally chase votes in the background to get PRs to > >>> merge.and, I > >>> > don't want to do that anymore. > >>> > > >>> > Amazing that you did that, but I agree that nagging is not a great > path > >>> > forward. > >>> > > >>> >> it is perfectly fine to
[jira] [Commented] (TINKERPOP-1815) Graph io requires the graph API
[ https://issues.apache.org/jira/browse/TINKERPOP-1815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16542847#comment-16542847 ] stephen mallette commented on TINKERPOP-1815: - I think we can close this one when TINKERPOP-1996 completes because graph providers can simply override the default behavior however they want. > Graph io requires the graph API > --- > > Key: TINKERPOP-1815 > URL: https://issues.apache.org/jira/browse/TINKERPOP-1815 > Project: TinkerPop > Issue Type: Improvement > Components: io >Affects Versions: 3.2.7 >Reporter: Bryn Cooke >Priority: Major > > Currently the io classes use the graph API to populate the graph. For graphs > that do not fully support the graph API means that standard IO won't work. > It would be great if io could use the traversal API for constructing the > graph. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
Re: [DISCUSS] Graph.io() deprecation
Just an update to say that I've made progress on this concept for https://issues.apache.org/jira/browse/TINKERPOP-1996 Still a few things left to do, but I have the changes in place and some tests and it works well. I did have to alter the API slightly to look more like: g.io('file.kryo').read() g.io('file.json').write() In this way, read()/write() behave more like termination steps which allows the use of with() as a configuration step to io() and enables self-iteration. I have this working for both OLTP and OLAP which means that TinkerPop has finally bridged that annoying usage gap and we should thus be able to close: https://issues.apache.org/jira/browse/TINKERPOP-550 For OLAP we basically replace the step created via g.io() with one that uses the CloneVertexProgram allowing users to alter the configurations (i.e. the Input/OutputFormat) for IO. There are still some odds and ends to work out, but so far this is looking pretty good. On Fri, Jun 29, 2018 at 2:13 PM Stephen Mallette wrote: > So, we made the change on: > > https://issues.apache.org/jira/browse/TINKERPOP-1985 > > to deprecate BLVP because we're going to delegate bulk loading to graph > providers. The issue I now see is that Graph.io() is also a form of bulk > loading which means it should probably go the route of deprecation if we're > following our thinking to the letter. But, I don't think that "bulk > loading" is the only reason to move on from Graph.io(). Here's some > problems with it from a user's perspective: > > 1. The API is terrifying - there are reasons why this API developed the > way that it did, but I won't dwell on them. The point is that it's > generally hard to use with all the Builder patterns layering in and out of > each other. This is especially true if you have to customize some aspect of > the configuration in some way. > 2. You can't use it with GLVs. > 3. It forces reliance on the Graph API which we try to quietly hide from > users and some graphs don't even support that aspect of the API directly > > From the graph provider perspective: > > 1. If you don't have a Graph API you have no way to support this which is > bad because users look at the TinkerPop docs, and see this io() capability > all over the place and it causes confusion > 2. You have no way to optimize the loading process. > > I won't even go into all the internal reasons I dislike Graph.io() - hehe > > So, after careful thought, I think that deprecation makes sense, however, > I don't think we want to completely lose the convenience of Graph.io(). > > "But you said that we would delegate all bulk loading to provider tools!" > > Well, yes, and I think that idea still holds with the caveat that in > providing this convenience we make it at least possible for a graph > provider to optimize on it. So, i'm proposing that we make "io()" a part of > GraphTraversalSource by adding: > > g.read(resource) > g.write(resource) > > where resource is a string representing a URL to some file location. We > might have other overloads here, but I'm not sure what those are yet. These > methods would essentially be new steps to the Gremlin language and thus > allow additional configurations to be passed using our new with() step. By > default, we can rely on OLTP style reading/writing so this will work for > any graph system. > > Now, graph providers already know what they would need to do here to > optimize - they would write a strategy to detect a ReadStep or WriteStep > and replace it with their own. An extraordinary example of this will fit in > perfectly with HadoopGraph. HadoopGraph should be able to have a strategy > that detects these steps and then replaces them with VertexProgramStep > containing a CloneVertexProgram. See what that means? We've just unified > Gremlin OLTP bulk read/write with OLAP. That's something we've been > thinking about for years. > > Another really cool thing - g.read() and g.write() will work with GLVs out > of the box! The only trick would be that the resource being loaded is > relative to the server not to the client. I really like that things work > out this way, because we further unify the scope of Gremlin functions under > a single syntax and further reduce the reliance on the ScriptEngine for > non-JVM languages. > > Just to be clear here - we really shouldn't be losing any functionality > with this change. I'd see us deprecating Graph.io() on the 3.4.0 line so it > would be a good long while before we tried to completely remove that method > and related infrastructure. > > Please let me know if there are any questions or concerns. If there are no > objections, I'll start to move forward in this direction and see what kinds > of problems I run into. > > > > > > > > > > > > > > > >
[jira] [Commented] (TINKERPOP-550) Gremlin IO needs to support both OLTP and OLAP naturally.
[ https://issues.apache.org/jira/browse/TINKERPOP-550?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16542826#comment-16542826 ] stephen mallette commented on TINKERPOP-550: We can close this one when TINKERPOP-1996 closes > Gremlin IO needs to support both OLTP and OLAP naturally. > - > > Key: TINKERPOP-550 > URL: https://issues.apache.org/jira/browse/TINKERPOP-550 > Project: TinkerPop > Issue Type: Improvement > Components: io >Affects Versions: 3.0.2-incubating >Reporter: Marko A. Rodriguez >Priority: Major > > {code:java} > g.io().writeKryo().submit(g.compute()) > {code} > ...something like that so its easy for people to use OLAP bulk writers and > readers that are coming down the line. right now {{Graph.io()}} is pretty > hardcoded to OLTP and generalizing it would be nice so its useful for > databases at scsale. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Updated] (TINKERPOP-1996) Introduce read() and write() steps
[ https://issues.apache.org/jira/browse/TINKERPOP-1996?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] stephen mallette updated TINKERPOP-1996: Description: Make {{read()}} and {{write()}} steps the replacements for {{graph.io()}} so that these operations are simply part of the Gremlin language. (was: Make {{g.read()}} and {{g.write()}} the replacements for {{graph.io()}} so that these operations are simply part of the Gremlin language.) > Introduce read() and write() steps > -- > > Key: TINKERPOP-1996 > URL: https://issues.apache.org/jira/browse/TINKERPOP-1996 > Project: TinkerPop > Issue Type: Improvement > Components: io, process >Affects Versions: 3.4.0 >Reporter: stephen mallette >Assignee: stephen mallette >Priority: Major > Labels: deprecation > Fix For: 3.4.0 > > > Make {{read()}} and {{write()}} steps the replacements for {{graph.io()}} so > that these operations are simply part of the Gremlin language. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[GitHub] tinkerpop issue #889: Tinkerpop 1977 - Sasl Authentication
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/889 We appreciate you taking the lead on this feature @mattallenuk . There is no rush, we can leave the pull request open and wait for any progress. ---