I don't know what others think but +1 from my side. Best, Stamatis
On Tue, Mar 17, 2020 at 9:16 AM JiaTao Tao <taojia...@gmail.com> wrote: > 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 <taojia...@gmail.com> 于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 <zabe...@gmail.com> 于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 <yuzhao....@gmail.com> > 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 <t...@apache.org>，写道： > >> > > 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 > >> > > >> > > >
