[jira] [Comment Edited] (CALCITE-5448) ReduceExpressionsRule does not always constant fold expressions

["Ruben Q L (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CALCITE-5448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17649869#comment-17649869 ] 

Ruben Q L edited comment on CALCITE-5448 at 12/20/22 4:16 PM:
--------------------------------------------------------------

[~julianhyde], I agree that is the general case. But in the particular case of {{ReduceExpressionsRule}}, if we look at the code that calls {{reduceExpressionsInternal}}, the {{RexSimply}} is built using either the executor from the planner, or as a fallback the "default" executor in RexUtil [1]:
{code}
    final RexExecutor executor = Util.first(cluster.getPlanner().getExecutor(), RexUtil.EXECUTOR);
    final RexSimplify simplify = new RexSimplify(rexBuilder, predicates, executor);
    final RexUnknownAs unknownAs = RexUnknownAs.falseIf(unknownAsFalse);
    final boolean reduced = reduceExpressionsInternal(rel, simplify, unknownAs,
        expList, predicates, treatDynamicCallsAsConstant);
{code}

IMO it seems a bit strange to use the {{RexUtil.EXECUTOR}} "fallback" in there, and not insider {{reduceExpressionsInternal}}.
Perhaps another alternative to avoid adding a new getter {{RexSimplify#getExecutor}} could be adding the {{RexExecutor executor}} as a parameter in {{reduceExpressionsInternal}}

[1] https://github.com/apache/calcite/blob/013f034dee3e24083760b4695e2eacfbf592c2cb/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java#L702


was (Author: rubenql):
[~julianhyde], I agree that is the general case. But in the particular case of {{ReduceExpressionsRule}}, if we look at the code that calls {{reduceExpressionsInternal}}, the {{RexSimply}} is built using either the executor from the planner, or as a fallback the "default" executor in RexUtil [1]:
{code}
    final RexExecutor executor = Util.first(cluster.getPlanner().getExecutor(), RexUtil.EXECUTOR);
    final RexSimplify simplify = new RexSimplify(rexBuilder, predicates, executor);

    // Simplify predicates in place
    final RexUnknownAs unknownAs = RexUnknownAs.falseIf(unknownAsFalse);
    final boolean reduced = reduceExpressionsInternal(rel, simplify, unknownAs,
        expList, predicates, treatDynamicCallsAsConstant);
{code}

IMO it seems a bit strange to use the {{RexUtil.EXECUTOR}} "fallback" in there, and not insider {{reduceExpressionsInternal}}.
Perhaps another alternative to avoid adding a new getter {{RexSimplify#getExecutor}} could be adding the {{RexExecutor executor}} as a parameter in {{reduceExpressionsInternal}}

[1] https://github.com/apache/calcite/blob/013f034dee3e24083760b4695e2eacfbf592c2cb/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java#L702

>  ReduceExpressionsRule does not always constant fold expressions
> ----------------------------------------------------------------
>
>                 Key: CALCITE-5448
>                 URL: https://issues.apache.org/jira/browse/CALCITE-5448
>             Project: Calcite
>          Issue Type: Bug
>          Components: core
>    Affects Versions: 1.32.0
>            Reporter: Mihai Budiu
>            Priority: Minor
>
> I have manually built a HepPlanner to optimize the SQL queries, and I discovered that the rule ReduceExpressionsRule does not really do anything in my setup.
> I am looking at method ReduceExpressionsRule.reduceExpressionsInternal.
> There is this piece of code:
> {code}
>     RexExecutor executor = rel.getCluster().getPlanner().getExecutor();
>     if (executor == null) {
>       // Cannot reduce expressions: caller has not set an executor in their
>       // environment. Caller should execute something like the following before
>       // invoking the planner:
>       //
>       // final RexExecutorImpl executor =
>       //   new RexExecutorImpl(Schemas.createDataContext(null));
>       // rootRel.getCluster().getPlanner().setExecutor(executor);
>       return changed;
>     }
> {code}
>  
> However, the caller of this function, the method reduceExpressions, has carefully inserted an executor in the RexSimplify class in case the cluster has no executor. Shouldn't that executor be used instead of trying the missing one? (It is currently private in the RexSimplify class.)
>  



--
This message was sent by Atlassian Jira
(v8.20.10#820010)