[jira] [Commented] (CALCITE-2736) ReduceExpressionsRule never reduces dynamic expressions but this should be configurable
[
https://issues.apache.org/jira/browse/CALCITE-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17613594#comment-17613594
]
Stamatis Zampetakis commented on CALCITE-2736:
--
As part of this change the following method was modified (a new parameter was
added) and during upgrade to 1.28.0 there were compilation errors cause there
were callers to this method. This is a breaking change and should be part of
the release notes.
{code:java}
protected static boolean reduceExpressions(RelNode rel, List expList,
RelOptPredicateList predicates, boolean unknownAsFalse,
boolean matchNullability)
{code}
Reminder: usually we follow the telescopic pattern; first deprecate the old
method and add new method with new parameter.
> 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
> Fix For: 1.28.0
>
> Time Spent: 2h
> 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.20.10#820010)
[jira] [Commented] (CALCITE-2736) ReduceExpressionsRule never reduces dynamic expressions but this should be configurable
[ https://issues.apache.org/jira/browse/CALCITE-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17412619#comment-17412619 ] Yingyu commented on CALCITE-2736: - [~julianhyde], any additional comment on my above changes? > 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)
[jira] [Commented] (CALCITE-2736) ReduceExpressionsRule never reduces dynamic expressions but this should be configurable
[
https://issues.apache.org/jira/browse/CALCITE-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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}
: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)
[jira] [Commented] (CALCITE-2736) ReduceExpressionsRule never reduces dynamic expressions but this should be configurable
[
https://issues.apache.org/jira/browse/CALCITE-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17402457#comment-17402457
]
Julian Hyde commented on CALCITE-2736:
--
The PR looks almost ready. A few comments:
* Expand the javadoc for the test methods. Something like "Tests that a
dynamic function (USER) is treated as a constant, and reduced, if \{link
ReduceExpressionsRule.Conig#treatDynamicCallsAsConstant} is false. Test case
for issue XXX."
* Javadoc for treatDynamicCallsAsConstant should say what the default is, and
give a brief example of the effect of the default.
* In the test, I don't think you need a new tester. You just need a new
executor, right? {{testReduceConstantsRequiresExecutor}} manages to set an
executor. Maybe you could call {{sql.withTransform}}
* The two test methods have quite a lot of common code. Can you figure out a
way to refactor them so that the common code is common and the differences are
apparent.
* Thanks for grouping the tests with other constant-reduction tests.
> 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: 1.5h
> 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)
[jira] [Commented] (CALCITE-2736) ReduceExpressionsRule never reduces dynamic expressions but this should be configurable
[ https://issues.apache.org/jira/browse/CALCITE-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17402309#comment-17402309 ] Yingyu commented on CALCITE-2736: - [~vladimirsitnikov]Thanks for your help in review and your comment. I agree with you that in general we should avoid negative naming. I've updated the naming using `treatDynamicCallsAsConstants` as suggested. Regarding the use case the direct usage would be to reduce query_user as part of planning despite it being a dynamic function. > 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: 1.5h > 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)
[jira] [Commented] (CALCITE-2736) ReduceExpressionsRule never reduces dynamic expressions but this should be configurable
[
https://issues.apache.org/jira/browse/CALCITE-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17401866#comment-17401866
]
Vladimir Sitnikov commented on CALCITE-2736:
I don't fully understand the use case (it would be sad to cache dynamic value
and get wrong results), however, the defaults are not changed, so I see no
issues.
The PR #980 looks good to me, except "rebase/squash" and the naming.
I would avoid naming fields and methods with {{Not}} since "non-constant" might
have multiple meanings.
> 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 20m
> 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)
[jira] [Commented] (CALCITE-2736) ReduceExpressionsRule never reduces dynamic expressions but this should be configurable
[ https://issues.apache.org/jira/browse/CALCITE-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17401856#comment-17401856 ] Yingyu commented on CALCITE-2736: - Since the previous PR (#980) didn't get a chance to merge into master I created this new PR by refactoring the original changes using current config API. https://github.com/apache/calcite/pull/2502 1. Add new config option treatDynamicCallsAsNonConstant. 2. Update ReducibleExprLocator.analyzeCall() method to also rely on treatDynamicCallsAsNonConstant option when determining whether to reduce a dynamic function. [~julianhyde], [~vladimirsitnikov], [~kgyrtkirk] could you help to review this PR since you reviewed the old PR in the past? > 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 10m > 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)
[jira] [Commented] (CALCITE-2736) ReduceExpressionsRule never reduces dynamic expressions but this should be configurable
[ https://issues.apache.org/jira/browse/CALCITE-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17401827#comment-17401827 ] Yingyu commented on CALCITE-2736: - Since the previous PRs (#973 and #980) didn't get a chance to merge into master I created this new PR by refactoring the changes using current config API. https://github.com/apache/calcite/pull/2502 1. Add new config option treatDynamicCallsAsNonConstant. 2. Update ReducibleExprLocator.analyzeCall() method to also rely on treatDynamicCallsAsNonConstant option when determining whether to reduce a dynamic function. [~julianhyde], [~vlsi], [~kgyrtkirk] could you help to review this PR since you reviewed the old PRs in the past? > 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 10m > 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)
[jira] [Commented] (CALCITE-2736) ReduceExpressionsRule never reduces dynamic expressions but this should be configurable
[ https://issues.apache.org/jira/browse/CALCITE-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16856628#comment-16856628 ] Michael Mior commented on CALCITE-2736: --- Any movement on this? Hoping to cut rc0 of 1.20.0 this week. > 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 > Fix For: 1.20.0 > > Time Spent: 1h > 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 (v7.6.3#76005)
[jira] [Commented] (CALCITE-2736) ReduceExpressionsRule never reduces dynamic expressions but this should be configurable
[ https://issues.apache.org/jira/browse/CALCITE-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16786814#comment-16786814 ] Kevin Risden commented on CALCITE-2736: --- Moving fix version to 1.20.0 since it looks like this change caused some issues for Hive and no new test case has been provided. PR is still outstanding as well. > 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 > Fix For: 1.19.0 > > Time Spent: 1h > 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 (v7.6.3#76005)
[jira] [Commented] (CALCITE-2736) ReduceExpressionsRule never reduces dynamic expressions but this should be configurable
[
https://issues.apache.org/jira/browse/CALCITE-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16723129#comment-16723129
]
Zoltan Haindrich commented on CALCITE-2736:
---
[~julianhyde], [~jnadeau]: accidentally I've used the master from github; which
still had this patch.
Note that with the current changes simplifications like:
{code}
- predicate: false (type: boolean)
+ predicate: ((year = 2001) and year is null) (type: boolean)
{code}
doesn't happen anymore - at least in Hive with the now deprecated
constructors...
I haven't gone into more detail; as I've just noticed that this will not be in
1.18...
> 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
> Fix For: 1.19.0
>
>
> 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
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2736) ReduceExpressionsRule never reduces dynamic expressions but this should be configurable
[
https://issues.apache.org/jira/browse/CALCITE-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16722578#comment-16722578
]
Jacques Nadeau commented on CALCITE-2736:
-
{quote}Regardless of which release it goes into, the change is producing
warnings relating to calls to deprecated APIs, so it shouldn't be in master. I
have put it onto a branch
[https://github.com/julianhyde/calcite/tree/2736-reduce-options] and
force-pushed to remove it from master. Let's merge it after the release, and
after the warnings are fixed.
{quote}
Can you share a link where this was decided (master should have no deprecation
warnings)? I'd like to review to better understand the thinking behind having
this rule but not enforcing in the build or pre-commit checks.
{quote}Policy is to release from master, not a spur. It produces a simple
change log, and holding back merges around release time is not too onerous for
a project of this velocity.
{quote}
I agree that a policy shouldn't be unilaterally dismissed without discussion.
That was definitely not my intent with this merge. The merge of Jesus's patch
without any discussion on the release thread was the second indicator that the
branch was open to me (beyond the lack of communication for some time on the
release thread). Per my comments above, there should be a clearer way to expose
this status. It would also preferably be time-bounded.
Side note: I disagree with your thoughts on the onerousness of this approach of
branch management. Two plus weeks of master lock is too long. I've hit this
several times in Calcite and I'd be surprised if I was alone. There is minimal
committer benefit of having a pure linear history for a few commit release spur
(even if you have to do a couple of cherry picks). I'll consider bringing this
up on the mailing list.
{quote}I don't want even minor changes to be added to this RC. Hive's regress
is in progress, and takes several days.
{quote}
It would have been nice to have clearer communication on the mailing list
around your goals with regards to the release and the timing. Until your post
today, your intentions, actions and timing weren't super clear.
> 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
> Fix For: 1.19.0
>
>
> 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
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2736) ReduceExpressionsRule never reduces dynamic expressions but this should be configurable
[ https://issues.apache.org/jira/browse/CALCITE-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16722532#comment-16722532 ] Jacques Nadeau commented on CALCITE-2736: - Hey [~julianhyde], sorry about that. I didn't realize 1.18 wasn't already out. I looked for the release thread and saw it wasn't active for 10 days or something and foolishly assumed that meant the release was done. What do you think is the best way to track that master is closed so others don't miss that the way I did? Also, how important is locking? Seems like having a little release spur would be fine. Another option is to include this patch in next release candidate. It doesn't change much except configurability of the one rule. > 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 > Fix For: 1.19.0 > > > 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 (v7.6.3#76005)
[jira] [Commented] (CALCITE-2736) ReduceExpressionsRule never reduces dynamic expressions but this should be configurable
[ https://issues.apache.org/jira/browse/CALCITE-2736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16722410#comment-16722410 ] Julian Hyde commented on CALCITE-2736: -- Since the master branch is currently closed (while 1.18 is released) I will force-push this change on top of the 1.18 release after the release is complete. > 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 > Fix For: 1.19.0 > > > 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 (v7.6.3#76005)
