Hi Oleksandr, Thanks for bringing this idea to the community. However, I would be very hesitant to include such a large change to TinkerPop. The Gremlin language doesn't really have a specification which means that what a Step does is more susceptible to change. I also feel like the current implementation of Steps wasn't designed to be a stable and open interface so I would consider them to be internal TinkerPop classes. From what I've heard, some of these classes have intentionally been marked as final to prevent misusage which is something that has happened in the past. Another concern I have is the increased potential to break other's changes during refactoring or bug fixing. It's easy to run into a situation where you make a fix for an issue but don't realize that you have accidentally broken someone else's implementation because you didn't know their code had a specific functional dependency on the current implementation. This scenario is much more likely to occur when you open up this many classes and there is no way to predict what others will do with it.
I think we can revisit this idea in the future once we have a firmer specification for the Gremlin language. For now I think it is better to take these changes on in a case by case basis. Thanks, Ken On Tue, Apr 18, 2023 at 6:27 PM Oleksandr Porunov < alexandr.poru...@gmail.com> wrote: > 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 >
