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
