Agree with James, and that's not even including implementations in other codebases that use calcite (e.g. there are dozens of implementations in Dremio's codebase).
On Thu, Dec 2, 2021 at 9:38 AM James Starr <[email protected]> wrote: > 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 < > [email protected]> 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 > > >
