Introduce new marker interfaces to identify whether a step can perform write or delete or both on graph data and static map to capture steps associated with an operator

[phanindhra876]

Currently all steps that can change graph data implements Mutating interface. However, there is no straight forward way to know whether the step can perform write or delete or both on the graph data. This PR introduces two new marker interfaces Writing and Deleting defining whether step can perform write or delete or both.

In addition to this, this PR creates a static map of Traversal steps that shall be added for a given operator. Both can be combined to assess whether a given query can perform read or write or delete or any combination of these before query execution.

[codecov-commenter]

Codecov Report

Merging #2025 (a56f8ad) into 3.6-dev (e5ef74b) will increase coverage by 0.09%.
The diff coverage is 98.40%.

@@              Coverage Diff              @@
##             3.6-dev    #2025      +/-   ##
=============================================
+ Coverage      69.44%   69.54%   +0.09%     
- Complexity      9347     9351       +4     
=============================================
  Files            878      878              
  Lines          42103    42228     +125     
  Branches        5643     5643              
=============================================
+ Hits           29239    29366     +127     
+ Misses         10869    10868       -1     
+ Partials        1995     1994       -1     

Impacted Files	Coverage Δ
...remlin/process/traversal/step/filter/DropStep.java	70.58% <ø> (ø)
...n/process/traversal/step/map/AddEdgeStartStep.java	84.21% <ø> (ø)
...remlin/process/traversal/step/map/AddEdgeStep.java	85.00% <ø> (ø)
...process/traversal/step/map/AddVertexStartStep.java	80.00% <ø> (ø)
...mlin/process/traversal/step/map/AddVertexStep.java	71.05% <ø> (ø)
.../gremlin/process/traversal/step/map/MergeStep.java	75.75% <ø> (ø)
...ess/traversal/step/sideEffect/AddPropertyStep.java	78.31% <ø> (ø)
...gremlin/process/traversal/util/BytecodeHelper.java	85.05% <98.40%> (+34.03%)	⬆️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[kenhuuu]

Thanks for contributing this. It may also be nice to add a section to the upgrade documentation for providers to let them know about this new feature.

VOTE +1

[vkagamlyk]

VOTE+1

[Cole-Greer]

Thanks @phanindhra876 for these changes. Overall they look good to me. My main question is if we've actually covered all the correct steps with these marker interfaces. By the look of it, this PR is just updating all of the steps which previously implemented the Mutating interface which in theory is sufficient. I wonder though if some consideration should be given into adding these markers to io() and possibly call() as well.

Also I wanted to ask if the findPossibleTraversalStepsForModulatorOperators() test is necessary as it appears to me that all of those modulators should already be tested by findPossibleTraversalSteps(). It is possible I'm misunderstanding this though so please correct me if I'm wrong.

[phanindhra876]

Intention of test findPossibleTraversalSteps is to check if we have covered all the symbols defined in the map or not. We don't do any checks whether the symbol is associated with any Step or not. To avoid over complicating the test, i have created another test to check for emptyness of ModulatingOperators.

[phanindhra876]

I wonder though if some consideration should be given into adding these markers to io() and possibly call() as well.

For the scope of this PR, i have updated steps extending Mutating to implement Writing or Deleting or both. However, IoStep and CallStep does not implement Mutating step and i need to spend some time to understand how to update them. Maybe that can be added in a separate PR? I will work with Stephen to figure this out.

[spmallette]

Writing and Deleting aren't really marker interface only, because Mutating isn't. Mutating brings with it the Event subsystem for EventStrategy. I don't think that will work well with io() or call(). I think that providers will just need to account for those steps as special cases rather than test them for Writing or Deleting. for example analyzing call() bytecode might require the additional check of examining the service being called for that step to determine its action. so, long story short, it was good to call those steps out, but i don't think they can implement any part of the Mutating interface the way it is defined. VOTE +1

[Cole-Greer]

Intention of test findPossibleTraversalSteps is to check if we have covered all the symbols defined in the map or not. We don't do any checks whether the symbol is associated with any Step or not. To avoid over complicating the test, i have created another test to check for emptyness of ModulatingOperators.

I see, I had missed that the second test is asserting an empty list. Those tests look good to me. I agree with Stephen's analysis of io() and call() as well. Thanks for submitting these changes, they look good to me. VOTE +1 (Non-binding).

[kenhuuu]

Do you mind squashing your commits and force pushing? Our current policy is to have the author do this rather than the person merging. Thanks.

[phanindhra876]

@kenhuuu Squashed all commits into one commit.

[saikiranboga]

lgtm

[xiazcy]

VOTE +1.
I've created a JIRA here for tacking purposes: https://issues.apache.org/jira/browse/TINKERPOP-2929.