What if RelNode.accept(RelShuttle) was deprecated and the RelShuttleImpl had an if(node instanceof Logical*Node) instead?
I am against further enhancing RelNode.accept(RelShuttle), because, as Vladimir pointed out, it does not actually follow a visitor pattern(aka walking the tree) and fixing that could be a breaking change. I would prefer if the tree traversal and the dispatch was independent of the RelNode implementation. This would help with loose coupling so we do not have email threads talking visit patterns when people like want to do something rather simple. James On Fri, Dec 3, 2021 at 1:11 PM Jacques Nadeau <jacq...@apache.org> wrote: > I think this thread has forked into two distinct topics: > > 1. A general discussion of the utility of visitors > 2. Support for generic return type for RelShuttle (the original purpose > of the thread) > > > My quick thoughts on #1: I've historically used the visitors in Calcite > extensively (external to Calcite and typically not using RelShuttleImpl > which requires an instance per user/thread due to internal use of a deque. > I don't support the removal of visitors or the current impl of accept from > RelNode. I think that would be disruptive to the community. > > For #2: It seems like most people are supportive of this kind of pattern > assuming it doesn't impact compatibility. I 100% agree on compat and am > exploring the best way to actually expose this safely. The problem I > haven't yet found a great solution for is if someone has built a new > relnode type that overrides the default RelShuttle based accept methods. In > this situation, to maintain compatibility, you can't actually delegate the > relshuttle use to the generic method (as you would skip over the > customization). The options I've identified: > > a) Have a (* instanceof RelShuttle) check in the generic path that routes > to the specific accept methods. > b) Use reflection to statically determine if a particular relnode class has > a relsthuttle based override and then route to it in those circumstances > (similar to (a) but more complex and likely slightly more performant). > c) Don't try to collapse the existing RelShuttle into a new pattern and > instead just introduce a new pattern independently. > d) Start with a or b but also deprecate the RelShuttle specific 'accept' > methods. Then in a subsequent release, remove the RelShuttle specific > methods entirely. > > I'm currently leaning towards starting with (a) since it is the easiest for > users to understand. However, I'd love to hear if others have better ideas > or opinions on these existing ideas. > > Thanks! > Jacques > > > > > On Thu, Dec 2, 2021 at 10:22 AM Vladimir Sitnikov < > sitnikov.vladi...@gmail.com> wrote: > > > Steven>Agree with James, and that's not even including implementations in > > other > > Steven>codebases that use calcite (e.g. there are dozens of > implementations > > in > > Steven>Dremio's codebase). > > > > Can you please show RelShuttle in Dremio codebase? > > I see no RelShuttle implementations: > > https://github.com/dremio/dremio-oss/search?q=relshuttle > > > > Note: RelShuttleImpl does not count since RelShuttleImpl does not really > > need RelNode cooperation. > > RelShuttleImpl just enumerates children of the relations, and it does not > > rely on accept(..) calls. > > > > It can do the very same thing without calling accept(this), so we can > > remove RelNode#accept(RelNode) methods, > > and we can still get the same RelShuttleImpl. > > It can have a series of if (... instanceof) checks or a > Map#get(getClass()) > > or even https://github.com/forax/exotic#visitor---javadoc > > > > Vladimir > > >
