Hi Danny Thanks for your reply, I think Stamatis Zampetakis's opinion is summative, and here the problem I think is a default RexExecutor is better than null, especially, in this case, cuz `reduceExpressionsInternal` and `reduceExpressions` is in the same path, thought the use of RexExecutor may be different, but it still makes people confusing.
IMHO, if "return RexUtil.EXECUTOR" >= "return null", we can do the modify. If you think so, I can open a JIRA and do this minor change. Hope to hear your voice. Regards! Aron Tao JiaTao Tao <[email protected]> 于2020年3月17日周二 下午4:02写道： > 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 >> > >> >
