getScopingInfo utility added #3118

vaibhavm99 · 2025-05-09T22:47:01Z

Added a utility function getScopingInfo() in all classes implementing Scoping and a method getScopingInfoForTraversal to get the labels needed by a traversal, which returns a Set of ScopingInfo class which contains the label and Pop info.

Cole-Greer · 2025-05-09T23:05:10Z

Hi @vaibhavm99, thanks for opening a PR! I should have time to give it a full review by Monday at the latest. Could I ask a bit about the motivation for the new getScopingInfo() method? The label functionality is mostly covered by getScopeKeys(), is the main objective to give some common interface for finding the Pop value of "select" type steps? Is this change in response to any difficulties with the current interface?

codecov-commenter · 2025-05-09T23:08:42Z

Codecov Report

Attention: Patch coverage is 70.64220% with 32 lines in your changes missing coverage. Please review.

Project coverage is 78.17%. Comparing base (cfd6889) to head (d2ec3a4).
Report is 258 commits behind head on master.

Files with missing lines	Patch %	Lines
...rocess/traversal/step/map/TraversalSelectStep.java	0.00%	8 Missing ⚠️
.../gremlin/process/traversal/step/PopContaining.java	80.00%	2 Missing and 2 partials ⚠️
...n/process/traversal/step/map/AddEdgeStartStep.java	0.00%	4 Missing ⚠️
...remlin/process/traversal/step/map/AddEdgeStep.java	0.00%	4 Missing ⚠️
...process/traversal/step/map/AddVertexStartStep.java	0.00%	4 Missing ⚠️
...mlin/process/traversal/step/map/AddVertexStep.java	0.00%	4 Missing ⚠️
...ess/traversal/step/sideEffect/AddPropertyStep.java	0.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3118      +/-   ##
============================================
+ Coverage     77.87%   78.17%   +0.30%     
- Complexity    13578    13799     +221     
============================================
  Files          1015     1025      +10     
  Lines         59308    60157     +849     
  Branches       6835     7016     +181     
============================================
+ Hits          46184    47030     +846     
+ Misses        10817    10779      -38     
- Partials       2307     2348      +41

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

vkagamlyk · 2025-05-10T17:41:34Z

Hi @vaibhavm99, will this change allow to save not the whole Path, but only the parts that are actually used?

vaibhavm99 · 2025-05-13T02:33:09Z

Hi @vaibhavm99, thanks for opening a PR! I should have time to give it a full review by Monday at the latest. Could I ask a bit about the motivation for the new getScopingInfo() method? The label functionality is mostly covered by getScopeKeys(), is the main objective to give some common interface for finding the Pop value of "select" type steps? Is this change in response to any difficulties with the current interface?

getScopeKeys only gives us a List of labels needed, but does not contain the Pop info. This is a common interface to get both label and the Pop value for all the labels needed. This is to provide label information inside the repeat step during the Neptune FastTranslation.

vaibhavm99 · 2025-05-13T02:37:17Z

Hi @vaibhavm99, will this change allow to save not the whole Path, but only the parts that are actually used?

Hi @vkagamlyk , I'm sorry I don't understand what you mean by "the whole Path." This change should not have any breaking problems with older code, as it is just adding a new API. This would take in a traversal and provide us with the label and Pop info for the labels that are needed in that traversal.

vkagamlyk · 2025-05-13T04:29:56Z

Hi @vaibhavm99, will this change allow to save not the whole Path, but only the parts that are actually used?

Hi @vkagamlyk , I'm sorry I don't understand what you mean by "the whole Path." This change should not have any breaking problems with older code, as it is just adding a new API. This would take in a traversal and provide us with the label and Pop info for the labels that are needed in that traversal.

Hi @vaibhavm99!
In some cases it is necessary to transfer Traversers between servers. Part of Traverser can be Path object that is larger than other Traverser parts. If we know in advance which parts of the path are used, then we can transfer only them.
Path is list of pairs <label, value>, getScopeKeys can be used to get list of used labels.

Cole-Greer · 2025-05-13T17:07:29Z

Hi @vaibhavm99 I'm happy with extending convenience interfaces for providers. I am curious since the SelectStep, SelectOneStep, and TraversalSelectStep are the only steps which are not hardcoded to Pop.last, if you also considered unifying these steps with some sort of Selecting interface which could extend Scoping and add public Pop getPop();

...n-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/FormatStep.java

Cole-Greer · 2025-05-13T17:15:03Z

I see this PR is currently targeting the master branch, this branch is currently accumulating changes for TinkerPop 4.0.0-beta.2, and ultimately TinkerPop 4. I would recommend targeting this change to 3.7-dev, which is accumulating changes for 3.7.4 (non-breaking only), or 3.8-dev which is for TinkerPop 3.8.0 (breaking changes permitted). Any changes introduced to an older development branch will also get introduced to the newer branches when the PR is merged.

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java

spmallette · 2025-05-13T17:44:52Z

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java

+     *
+     * @return A Set of {@link ScopingInfo} values which contain the label and Pop value
+     */
+    public default Set<ScopingInfo> getScopingInfo() {return null; }


Should there be a default implementation here? If something implements Scoping shouldn't it have a proper implementation? or are there cases where you would return getScopeKeys but not getScopingInfo? and if so, would return null be the right default?

Yes, there are a few classes that do not implement Pop at all, so for them we do not need getScopingInfo() but they do have getScopeKeys(). For example, AddEdgeStep and AddPropertyStep. For those I added a default. Would changing it to return an empty Set be better?

vaibhavm99 · 2025-05-13T20:10:09Z

Hi @vaibhavm99, will this change allow to save not the whole Path, but only the parts that are actually used?

Hi @vkagamlyk , I'm sorry I don't understand what you mean by "the whole Path." This change should not have any breaking problems with older code, as it is just adding a new API. This would take in a traversal and provide us with the label and Pop info for the labels that are needed in that traversal.

Hi @vaibhavm99! In some cases it is necessary to transfer Traversers between servers. Part of Traverser can be Path object that is larger than other Traverser parts. If we know in advance which parts of the path are used, then we can transfer only them. Path is list of pairs <label, value>, getScopeKeys can be used to get list of used labels.

This API would not cause any breaking change in getScopeKeys, this would give us the Pop info for those labels as well if you'd need it. When some labels are overloaded, Pop can be used to distinguish which mapping of that label is needed. For example, if only first is needed we would just need to have the <label, value1> instead of all the <label, value> pairs. This API gives additional info along with what getScopeKeys provides.

vaibhavm99 · 2025-05-13T20:13:29Z

Hi @vaibhavm99 I'm happy with extending convenience interfaces for providers. I am curious since the SelectStep, SelectOneStep, and TraversalSelectStep are the only steps which are not hardcoded to Pop.last, if you also considered unifying these steps with some sort of Selecting interface which could extend Scoping and add public Pop getPop();

Yes, getScopingInfo() is overloaded in every class that implements Scoping, if that class hardcodes Pop to Pop.last that is given out, and if it gives the user option to select Pop, I used that value using the getPop() function.

upadhyay-prashant · 2025-05-13T22:15:57Z

@vkagamlyk / @Cole-Greer ,

Thank you for the detailed review. You got it right, that we're working on optimizing label identification in prefix traversals, with the goal of only injecting necessary labels rather than the entire path. This approach could indeed be valuable for Traverser injection in TinkerPop.

You've made a good point about the current limitations of the Scoping interface, particularly regarding complete label information requirements. Let me explain why the current POJO structure might be more suitable than the proposed alternative:

Consider this example:

gremlin> g.V().as('a').
......1>   out().as('b', 'a').
......2>   in().as('b').
......3>   where(select(first, 'a').where(eq('b')))

==>v[1]
==>v[1]
==>v[1]
==>v[4]
==>v[4]

In this case, the outermost where step requires [['a',Pop.first], ['b',Pop.last]], which wouldn't fit well with the proposed structure:

public static class ScopingInfo {
    public Set<String> labels;
    public Pop pop;
}

@spmallette : I hope above example explains why an simple getPop API wont be enough. Let me know if you have better suggestions.

Regarding the implementation strategy, our idea was to merge to master first and then cherry-pick to 3.7 and 3.8. Would you like to suggest any specific concerns about this approach?

Looking forward to your thoughts on this.

Cole-Greer · 2025-05-14T18:14:57Z

Hi @upadhyay-prashant, I'm not sure I understand the limitation arising from the above example with a simple getPop() API.

In the case of where(select(first, 'a').where(eq('b'))), you should be able to get all of the relevant data via:

selectStep.getScopeKeys() -> {'a'}
selectStep.getPop() -> Pop.first
whereStep.getScopeKeys() -> {'b'}
whereStep.getPop() -> Pop.last

There is never more than 1 Pop in a single step instance, I don't see why the same data as getScopingInfo() cannot be extracted by a call to getScopeKeys() and getPop().

Cole-Greer · 2025-05-14T18:17:17Z

Regarding the implementation strategy, our idea was to merge to master first and then cherry-pick to 3.7 and 3.8. Would you like to suggest any specific concerns about this approach?

The typical approach for this in TinkerPop would be to target the initial PR to 3.7-dev. Whichever committer ends up merging the PR will then manually merge the changes up to 3.8-dev and master on your behalf. Typically the only time we ask contributors to open separate PRs for each branch is if each branch requires substantially different solutions for some reason.

and TraversalParent.java, and classes that use these interfaces.

xiazcy · 2025-05-23T23:11:09Z

LGTM, thanks for the change! VOTE +1

Cole-Greer

Overall it looks good to me. Left a couple of comments for small tweaks. There are a couple of steps which give their own implementation for getPopInstructions() which do not have tests in this PR, which would be nice to have (TraversalSelectStep, AddVertexStep, AddVertexStartStep, AddEdgeStep, AddEdgeStartStep). It would also be nice to have some testing for steps which directly use the default implementations in Scpoing or TraversalParent.

For all of the steps which are TraversalParents, it would be nice if the tests also validated that getPopInstructions() is successfully producing the PopInstruction's from child traversals, as well as from the step itself.

Cole-Greer · 2025-05-26T17:13:26Z

...in-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/PopContaining.java

+         * - label: String
+         * - pop: Pop value
+         */
+        class PopInstruction {


Nit: this child class is indented 4 spaces too deep

Cole-Greer · 2025-05-26T17:16:03Z

...in-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/PopContaining.java

+ * @author Vaibhav Malhotra
+ */
+public interface PopContaining {
+    public default HashSet<PopInstruction> getPopInstructions() {


I'm not sure if we want a default implementation which returns an empty set. Are there any steps which will utilize this default behaviour?

If everyone is expected to override this method to return useful/accurate results, I would prefer not to have any default implementation.

Cole-Greer · 2025-05-26T18:44:30Z

gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/TestDataBuilder.java

+        for (Object[] pair : pairs) {
+            if (pair.length == 2 && pair[0] instanceof String && pair[1] instanceof Pop) {
+                popInstructions.add(new PopContaining.PopInstruction((String)pair[0], (Pop)pair[1]));
+            }


Nit: Would be good to add an else which throws an IllegalArgumentException to identify misuse of this helper.

Cole-Greer · 2025-05-26T18:59:22Z

...re/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/FormatStepTest.java

+        final PopContaining.PopInstruction popInstruction1 = new PopContaining.PopInstruction();
+        popInstruction1.label = "Hello";
+        popInstruction1.pop = Pop.last;
+
+        final PopContaining.PopInstruction popInstruction2 = new PopContaining.PopInstruction();
+        popInstruction2.label = "world";
+        popInstruction2.pop = Pop.last;
+
+        final HashSet<PopContaining.PopInstruction> popInstructionSet = new HashSet<>();
+        popInstructionSet.add(popInstruction1);
+        popInstructionSet.add(popInstruction2);
+


Nit: Could use TestDataBuilder.createPopInstructionSet()

Cole-Greer · 2025-05-26T20:56:23Z

...re/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectStepTest.java

@@ -63,4 +67,50 @@ public void shouldRequirePathsAccordingly() {
            assertEquals(traversalPath[0], ((Traversal.Admin<?, ?>) traversalPath[1]).getTraverserRequirements().contains(TraverserRequirement.LABELED_PATH));
        }
    }
+
+    @Test
+    public void testPopInstruction() {


Would be nice to also add equivalent tests to these for TraversalSelectStep

Cole-Greer reviewed May 13, 2025

View reviewed changes

...n-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/FormatStep.java Outdated Show resolved Hide resolved

spmallette reviewed May 13, 2025

View reviewed changes

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java Outdated Show resolved Hide resolved

spmallette reviewed May 13, 2025

View reviewed changes

Added PopContaining interface, and its implementation in Scoping.java

d2ec3a4

and TraversalParent.java, and classes that use these interfaces.

vaibhavm99 force-pushed the getScopeInfo_utility branch from 277612e to d2ec3a4 Compare May 22, 2025 23:40

Cole-Greer reviewed May 26, 2025

View reviewed changes

getScopingInfo utility added #3118

Are you sure you want to change the base?

getScopingInfo utility added #3118

Uh oh!

Conversation

vaibhavm99 commented May 9, 2025

Uh oh!

Cole-Greer commented May 9, 2025

Uh oh!

codecov-commenter commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vkagamlyk commented May 10, 2025

Uh oh!

vaibhavm99 commented May 13, 2025

Uh oh!

vaibhavm99 commented May 13, 2025

Uh oh!

vkagamlyk commented May 13, 2025

Uh oh!

Cole-Greer commented May 13, 2025

Uh oh!

Uh oh!

Cole-Greer commented May 13, 2025

Uh oh!

Uh oh!

spmallette May 13, 2025

Choose a reason for hiding this comment

Uh oh!

vaibhavm99 May 13, 2025

Choose a reason for hiding this comment

Uh oh!

vaibhavm99 commented May 13, 2025

Uh oh!

vaibhavm99 commented May 13, 2025

Uh oh!

upadhyay-prashant commented May 13, 2025

Uh oh!

Cole-Greer commented May 14, 2025

Uh oh!

Cole-Greer commented May 14, 2025

Uh oh!

xiazcy commented May 23, 2025

Uh oh!

Cole-Greer left a comment

Choose a reason for hiding this comment

Uh oh!

Cole-Greer May 26, 2025

Choose a reason for hiding this comment

Uh oh!

Cole-Greer May 26, 2025

Choose a reason for hiding this comment

Uh oh!

Cole-Greer May 26, 2025

Choose a reason for hiding this comment

Uh oh!

Cole-Greer May 26, 2025

Choose a reason for hiding this comment

Uh oh!

Cole-Greer May 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented May 9, 2025 •

edited

Loading