Hi Stamatis Zampetakis I agree with this completely: "The API of RexExecutor says the following "If an expression cannot be reduced, writes the original expression..." so we don't break anything by providing a default one."
Regards! Aron Tao Stamatis Zampetakis <[email protected]> 于2020年3月16日周一 下午9:52写道： > Interestingly, I was looking at this same piece of code not so long ago and > I agree it is a bit confusing. > > Looking around the places that we obtain a RexExecutor, most often > (always?) we observe the following pattern: > > RexExecutor executor = > Util.first(query.getCluster().getPlanner().getExecutor(), > RexUtil.EXECUTOR); > > I think it is always useful to have an executor in the planner thus I am > tempted to change the API of RelOptPlanner#getExecutor to always return an > (default) executor if an explicit one is not set. > > The API of RexExecutor says the following "If an expression cannot be > reduced, writes the original expression..." so we don't break anything by > providing a default one. > > What do you think? > > Best, > Stamatis > > On Mon, Mar 16, 2020 at 11:11 AM Danny Chan <[email protected]> wrote: > > > Thanks, the code is a little mess, here is how I understand it: > > > > The executor from `final RexExecutor executor > > = Util.first(cluster.getPlanner().getExecutor(), RexUtil.EXECUTOR)` is > > mainly used to construct the RexSimplify, in the RexSimplify, the > > expression that we evaluate is what we can make sure RexUtil.EXECUTOR can > > resolve(if you check the code, it only reduce the literals). > > > > But the expressions in the ReduceExpressionsRule may be more complex, > > somehow we must relay on the engine to plugin their RexExecutor to make a > > constant reduction(some engine use code generation, some use Java > > reflection). > > > > So, in total, the executor in RexSimplify has a fallback is because it’s > > expression to reduce is simple enough. > > > > Best, > > Danny Chan > > 在 2020年3月16日 +0800 PM3:57，JiaTao Tao <[email protected]>，写道： > > > In method reduceExpressionsInternal, we get RexExecutor from cluster, > it > > can be null: > > > <> > > > > > > But in the outside(reduceExpressions), `final RexExecutor executor = > > Util.first(cluster.getPlanner().getExecutor(), RexUtil.EXECUTOR)`, it > can't > > be null. > > > > > > And reduceExpressions is the only caller of reduceExpressionsInternal, > > so I think this is an inconsistent behavior. > > > > > > IMHO, we should create RexUtil.EXECUTOR if it is null > > in reduceExpressionsInternal, or just get RexExecutor from RexSimplify. > > > <> > > > > > > Regards! > > > Aron Tao > > >
