Re: [DISCUSS] get RexExecutor from RexSimplify in method reduceExpressionsInternal
IMHO, A disadvantage of supplying a default RexExecutor is that we cannot make sure that the reduced result is the same as the result of the execution engine, especially when there is some customized implementation. Best, Chunwei On Fri, Mar 20, 2020 at 11:16 AM Danny Chan wrote: > 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 ，写道： > > 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 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 ，写道： > > > > 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 于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 于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
Re: [DISCUSS] get RexExecutor from RexSimplify in method reduceExpressionsInternal
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 ，写道： > 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 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 ，写道： > > > 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 于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 于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 > > 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 ，写道： > > > > > > > In method reduceExpressionsInternal, we get RexExecutor from > > cluster, > > > > > it > > > > > >
Re: [DISCUSS] get RexExecutor from RexSimplify in method reduceExpressionsInternal
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 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 ，写道： > > 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 于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 于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 > 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 ，写道： > > > > > > 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
Re: [DISCUSS] get RexExecutor from RexSimplify in method reduceExpressionsInternal
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 ，写道： > 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 于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 于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 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 ，写道： > > > > > 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 > > > > > > > > >
Re: [DISCUSS] get RexExecutor from RexSimplify in method reduceExpressionsInternal
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 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 于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 于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 > 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 ，写道： > >> > > 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 > >> > > >> > > >
Re: [DISCUSS] get RexExecutor from RexSimplify in method reduceExpressionsInternal
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 于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 于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 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 ，写道： >> > > 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 >> > >> >
Re: [DISCUSS] get RexExecutor from RexSimplify in method reduceExpressionsInternal
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 于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 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 ，写道： > > > 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 > > >
Re: [DISCUSS] get RexExecutor from RexSimplify in method reduceExpressionsInternal
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 ，写道： > 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
[DISCUSS] get RexExecutor from RexSimplify in method reduceExpressionsInternal
In method reduceExpressionsInternal, we get RexExecutor from cluster, it can be null: [image: image.png] 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. [image: image.png] Regards! Aron Tao
