Vladimir, there are 22 implementations of RelShuttleImpl(excluding tests) in calcite core. That hardly seems like no value add. I am not sure why RelNode.visit needs to exist though, the dispatch could simply be done in some method internal to the visitor/shuttle.
On Thu, Dec 2, 2021 at 2:07 AM Vladimir Sitnikov < sitnikov.vladi...@gmail.com> wrote: > I've no issues with making the existing visitor more generic, however, > the choice of accept methods seems arbitrary :-/ > > Would it help if we move or prefer using algebras rather than visitors? > > https://oleksandrmanzyuk.wordpress.com/2014/06/18/from-object-algebras-to-finally-tagless-interpreters-2/ > > Just to clarify: > 1) I would like to refrain from breaking compatibility, especially if we > add an abstract method that is used in 1% of Calcite calls. > If you still want to break backward compatibility, it would be great if you > could show at least one use case for the change. > Otherwise, it would end up a breakage without even having a use case for it > :-( > No kidding. I think RelShuttle is not really useful, and we could just drop > all RelNode#accept methods and lose virtually no features. > Adding generics on top of RelShuttle is not going to heal that. > > 2) The current RelShuttle does not seem to add much value. > In other words, all implementations of accept(RelShuttle) method are > trivial. > The example from Wikipedia suggests: > > https://en.wikipedia.org/wiki/Visitor_pattern#What_solution_does_the_Visitor_design_pattern_describe > ? > > Wikipedia>Define a separate (visitor) object that implements an operation > to be performed on elements of an object structure > Wikipedia>*Clients* *traverse *the object structure and *call a dispatching > operation accept* (visitor) on an element > > I think the naming or the implementation in Calcite is wrong. > We should either stop naming such things as RelShuttle and RexVisitor > "visitors" or we should incline to the canonical approach. > > Vladimir >
