[
https://issues.apache.org/jira/browse/CALCITE-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17403409#comment-17403409
]
Yingyu commented on CALCITE-2736:
---------------------------------
Hi [~julianhyde], many thanks for your review!
I've updated the PR #2502 and addressed most of your comments above.
Regarding the tester issue with the test. Yes, I just need a new executor.
However, I found it's a bit tricky to set a customized executor:
- The way {{testReduceConstantsRequiresExecutor}} used doesn't work. That's
because during {{sql.check()}} we have following calling stack:
{noformat}
<init>:602, SqlToRelTestBase$TesterImpl (org.apache.calcite.test)
withConfig:919, SqlToRelTestBase$TesterImpl (org.apache.calcite.test)
lambda$withConfig$1:277, RelOptTestBase$Sql (org.apache.calcite.test)
apply:-1, 1681699484 (org.apache.calcite.test.RelOptTestBase$Sql$$Lambda$488)
check:335, RelOptTestBase$Sql (org.apache.calcite.test)
{noformat}
As you can see a new {{TesterImpl}} instance is constructed and
{{tester.planner}} becomes {{null}}. So whatever {{planner}} and {{executor}}
have been set before by
{code:java}
tester.convertSqlToRel("values 1").rel.getCluster().getPlanner()
.setExecutor(executor);
{code}
are lost. When {{planner}} is required later, it will be initialized by:
{{getPlanner()}} -> {{createPlanner()}} ->
{{plannerFactory.apply(getContext())}}. By default, {{plannerFactory}} is
initialized to {{MockRelOptPlanner::new}}. So we'll end up having an executor
with EMPTY context:
{code:java}
public MockRelOptPlanner(Context context) {
super(RelOptCostImpl.FACTORY, context);
setExecutor(new RexExecutorImpl(DataContexts.EMPTY));
}
{code}
So {{testReduceConstantsRequiresExecutor}} doesn't actually test with a nulll
executor but with an executor with EMPTY context. Although it doesn't affect
the test result.
- I used a customized {{plannerFactory}} to work around above problem.
- I've refectored the code a little bit to avoid using an explicit new tester:
{code:java}
.withTester(t -> ((TesterImpl) tester).withPlannerFactory(context -> planner))
{code}
Please let me know if this is ok or there there are better ways.
Thanks.
> ReduceExpressionsRule never reduces dynamic expressions but this should be
> configurable
> ---------------------------------------------------------------------------------------
>
> Key: CALCITE-2736
> URL: https://issues.apache.org/jira/browse/CALCITE-2736
> Project: Calcite
> Issue Type: Improvement
> Components: core
> Reporter: Jacques Nadeau
> Assignee: Jacques Nadeau
> Priority: Major
> Labels: pull-request-available
> Time Spent: 1h 50m
> Remaining Estimate: 0h
>
> There are situations where it is helpful to reduce dynamic SqlCalls. Right
> now, the ReduceExpressionsRule always avoids doing this. We should enhance
> the rule so this can be configurable depending on where in planning the rule
> is used.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
