why do We use inner class for CallCopyingArgHandler within SqlShuttle rather static nested class as ArgHandlerImpl within SqlBasicVisitor.

[Yanjing Wang <zh...@gmail.com>]

In my scenarios, I created some visitors for sql node to rewrite the sql,
for example, rewriting all literals to generate a sql template.

But I don't like many new constructors scattered somewhere. I like
singleton and dependency injection. so I read SqlShuttle source code to
look for the probability.

I find the CallCopyingArgHandler is not static, but the doc doesn't specify
the reason.
Does someone know why?

Re: why do We use inner class for CallCopyingArgHandler within SqlShuttle rather static nested class as ArgHandlerImpl within SqlBasicVisitor.

[Yanjing Wang <zh...@gmail.com>]

Julian is right that we still will create many CallCopyingArgHandler
instances though SqlShuttle is a sington.
I created a subclass QueryTemplatizer which extends SqlShuttle but
overrides visit(Literal or Interval) methods only, I make it a singleton in
DI context so that I don't need to create it explicitly.
This is just my case, I don't know whether others have or not.
Another consideration is that inner classes have the risk of memory leak
when others use elsewhere incorrectly, I always avoid using them unless it
has a private scope.

Vladimir Sitnikov <si...@gmail.com> 于2021年6月26日周六 上午2:37写道：

> Julian>I’m not a fan of dependency injection. .... But it requires mutable
> fields.
>
> On contrary, DI does not require mutable fields, and field injection is
> considered an anti-pattern in production code.
> For example, see
> https://github.com/google/guice/wiki/Injections#constructor-injection
>
> I do not see how DI could help with SqlShuttle though.
>
> Vladimir
>

Re: why do We use inner class for CallCopyingArgHandler within SqlShuttle rather static nested class as ArgHandlerImpl within SqlBasicVisitor.

[Vladimir Sitnikov <si...@gmail.com>]

Julian>I’m not a fan of dependency injection. .... But it requires mutable
fields.

On contrary, DI does not require mutable fields, and field injection is
considered an anti-pattern in production code.
For example, see
https://github.com/google/guice/wiki/Injections#constructor-injection

I do not see how DI could help with SqlShuttle though.

Vladimir

Re: why do We use inner class for CallCopyingArgHandler within SqlShuttle rather static nested class as ArgHandlerImpl within SqlBasicVisitor.

[Julian Hyde <jh...@gmail.com>]

> I find the CallCopyingArgHandler is not static, but the doc doesn't specify
> the reason.
> Does someone know why?

If you try making it static you will see that it is unable to use the current shuttle to visit the children.

CallCopyingArgHandler exists for a very short duration - visit of one SqlCall by the shuttle.

Still, there are multiple CallCopyingArgHandler instances active at the same time, so having one CallCopyingArgHandler per Shuttle instance (shared among all call visits) is not an option.

> On Jun 25, 2021, at 6:00 AM, Yanjing Wang <zh...@gmail.com> wrote:
> 
> In my scenarios, I created some visitors for sql node to rewrite the sql,
> for example, rewriting all literals to generate a sql template.
> 
> But I don't like many new constructors scattered somewhere. I like
> singleton and dependency injection. so I read SqlShuttle source code to
> look for the probability.

I’m not a fan of dependency injection. I can see that it has its place, e.g. for gluing together components at the macro level. But it requires mutable fields. At the micro level, I am a fan of immutability, final fields assigned in the constructor.

I agree that CallCopyingArgHandler is a strange bird. It has the purpose of a “strategy pattern”, except that it has some state. It was written a long time ago, and I’m not sure I’d do the same today. (I haven’t given it much thought.)

Julian