This is a preference, I would prefer the default value to not throw exceptions.
Best, Danny Chan 在 2020年3月18日 +0800 PM3:53，Stamatis Zampetakis <[email protected]>，写道： > If a Janino exception comes up then it is a bug that we have to fix since > it violates the contract of the interface. > > From my point of view the modification is meaningful for two reasons: > * improves code readability; > * avoids confusing behavior where the rules for performing > constant reduction are present but this does not really happen (because > there is an executor missing). > > I would say that in production, if the engine does not want to perform > constant reduction, it is equally easy to not register the respective rules. > > Best, > Stamatis > > On Wed, Mar 18, 2020 at 3:29 AM Danny Chan <[email protected]> wrote: > > > I’m a little worried about it the default RexExecutorImpl can handle all > > the downstream projects expressions, and very probably not, there would be > > some Janino compile exception if it can not translate the RexNodes > > correctly. > > > > So strictly to say, change the RexExecutor to a default implementation may > > break something. I think it’s better if we have a real case to illustrate > > that the modification is meaningful. > > > > In production, if an engine really wants to support constant reduction for > > their all kinds of expression, they should set up the RexExecutor > > explicitly. If they do not set up that, the constant reduction just not > > happens, it is better than supplying a default RexExecutor but does not > > really work for all expression. > > > > So I’m +0 for this. > > > > Best, > > Danny Chan > > 在 2020年3月17日 +0800 PM4:16，JiaTao Tao <[email protected]>，写道： > > > 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 > > > > > > > > > > > > > > > > >
