The visitor pattern is not something written in stone. We all have a
general understanding of what a visitor is and how it is used but there are
also widely used variations.
The following snippet (referring to the visitor pattern) is taken from
"Design Patterns: Elements of Reusable Object-Oriented Software", the first
reference of Wikipedia and a book I like a lot.
"Who is responsible for traversing the object structure? A visitor must
visit each
element of the object structure. The question is, how does it get there? We
can
put responsibility for traversal in any of three places: in the object
structure,
in the visitor, or in a separate iterator object (see Iterator (257))."
There is not really right or wrong here; there are alternatives with
different advantages/disadvantages.
I like the idea of having a generic return type for the shuttle, I think it
is useful.
I think we should take the opportunity to see how we can make the new
API useful for the vast majority of use-cases.
Ideally, I would like to see a solution that can effectively replace both
RelVisitor and RelShuttle interfaces.
I am not saying remove RelVisitor, and RelShuttle directly but it would be
nice if we were able to deprecate them and remove them in a few releases
by providing a better alternative.
Both RelVisitor, and RelShuttle, have been in the code base for quite some
time now and they have many extensions both in the Calcite repo and in
other projects using Calcite.
I would suggest spending a bit of time on these extensions and see what
other makes sense to introduce in the new API.
I am planning to go again over the extensions of RelVisitor/RelShuttle in
Calcite and Hive and see if there are other changes that may make sense to
incorporate in the new design.
I would encourage others to do the same.
A few things that I had in mind at some point regarding the design of the
visitor are:
use default methods to avoid forcing everybody to implement if not
necessary;
do not rely on method overloading;
base dispatching on abstract relational expressions and not logical ones.
Example
Instead of:
RelNode visit(LogicalFilter filter);
use:
default <T extends Filter> R visitFilter(T filter) {
return null;
}
and other similar things but in order to get this right I think we have to
work out some concrete examples and rework existing use-cases.
The object-algebra approach shared by Vladimir sounds interesting but again
we should evaluate the existing use-cases and see what we really need.
Many thanks Jacques for taking this initiative, and all the rest for the
valuable feedback. I really think it is worth pushing this forward and
getting feedback is important to get this right.
Best,
Stamatis
On Fri, Dec 3, 2021 at 10:30 PM James Starr <[email protected]> wrote:
> 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 <[email protected]> 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 <
> > [email protected]> 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
> > >
> >
>
