[jira] [Commented] (CALCITE-2730) RelBuilder simplifies a filter with duplicate conjunction to empty
[ https://issues.apache.org/jira/browse/CALCITE-2730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16716889#comment-16716889 ] Stamatis Zampetakis commented on CALCITE-2730: -- I pushed a new commit replacing based on index. Each time, I am replacing only the first matching occurence (based on equals). Since the proccessRange method is called once per term we should be safe with this modif. > RelBuilder simplifies a filter with duplicate conjunction to empty > -- > > Key: CALCITE-2730 > URL: https://issues.apache.org/jira/browse/CALCITE-2730 > Project: Calcite > Issue Type: Bug > Components: core >Affects Versions: next >Reporter: Stamatis Zampetakis >Assignee: Julian Hyde >Priority: Major > Fix For: 1.18.0 > > > Regression from 1.17. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2730) RelBuilder simplifies a filter with duplicate conjunction to empty
[ https://issues.apache.org/jira/browse/CALCITE-2730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16715326#comment-16715326 ] Julian Hyde commented on CALCITE-2730: -- I don't think we should use reference equality. I think we should replace based on index. Here is the rationale. Since RexNodes are immutable, we encourage people to share them (i.e. use them multiple times in the same expression or plan). So it's possible to get AND(e1, e2, e1) where e1 are literally the same object, but we'd only like to remove the second occurrence of e1. > RelBuilder simplifies a filter with duplicate conjunction to empty > -- > > Key: CALCITE-2730 > URL: https://issues.apache.org/jira/browse/CALCITE-2730 > Project: Calcite > Issue Type: Bug > Components: core >Affects Versions: next >Reporter: Stamatis Zampetakis >Assignee: Julian Hyde >Priority: Major > Fix For: 1.18.0 > > > Regression from 1.17. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2730) RelBuilder simplifies a filter with duplicate conjunction to empty
[
https://issues.apache.org/jira/browse/CALCITE-2730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16715225#comment-16715225
]
Stamatis Zampetakis commented on CALCITE-2730:
--
Even before the introduction of Collections#replaceAll the method that was used
to replace a term in various places was:
{code:java}
terms.set(terms.indexOf(e), ...)
{code}
which was again based on reference equality ('=='). In other places, where
value based equality was required (before the changes by [~kgyrtkirk]) it was
based on RexNode#toString.
I pushed a commit reverting back to reference equality in a couple of places
and tests seem to pass fine. Do you see an immediate problem with this change?
> RelBuilder simplifies a filter with duplicate conjunction to empty
> --
>
> Key: CALCITE-2730
> URL: https://issues.apache.org/jira/browse/CALCITE-2730
> Project: Calcite
> Issue Type: Bug
> Components: core
>Affects Versions: next
>Reporter: Stamatis Zampetakis
>Assignee: Julian Hyde
>Priority: Major
> Fix For: 1.18.0
>
>
> Regression from 1.17.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2730) RelBuilder simplifies a filter with duplicate conjunction to empty
[ https://issues.apache.org/jira/browse/CALCITE-2730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16715176#comment-16715176 ] Zoltan Haindrich commented on CALCITE-2730: --- without equals being fully operable: equals() acted as a == ...I think replaceAll was just an easy way to write the replacement instruction > RelBuilder simplifies a filter with duplicate conjunction to empty > -- > > Key: CALCITE-2730 > URL: https://issues.apache.org/jira/browse/CALCITE-2730 > Project: Calcite > Issue Type: Bug > Components: core >Affects Versions: next >Reporter: Stamatis Zampetakis >Assignee: Julian Hyde >Priority: Major > Fix For: 1.18.0 > > > Regression from 1.17. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2730) RelBuilder simplifies a filter with duplicate conjunction to empty
[ https://issues.apache.org/jira/browse/CALCITE-2730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16715130#comment-16715130 ] Stamatis Zampetakis commented on CALCITE-2730: -- Before the change of [~kgyrtkirk] replaceAll at line 1873 (and other parts) was based on reference equality. I am wondering if this was a bug or really the intended behavior. I've noticed that even before the modifications from [~julianhyde] the previous code was also relying on reference equality. > RelBuilder simplifies a filter with duplicate conjunction to empty > -- > > Key: CALCITE-2730 > URL: https://issues.apache.org/jira/browse/CALCITE-2730 > Project: Calcite > Issue Type: Bug > Components: core >Affects Versions: next >Reporter: Stamatis Zampetakis >Assignee: Julian Hyde >Priority: Major > Fix For: 1.18.0 > > > Regression from 1.17. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2730) RelBuilder simplifies a filter with duplicate conjunction to empty
[ https://issues.apache.org/jira/browse/CALCITE-2730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16715080#comment-16715080 ] Zoltan Haindrich commented on CALCITE-2730: --- I wanted to suggest to prune duplicates prior to range based approach - and not to remove the interpretation of earlier conditions. > RelBuilder simplifies a filter with duplicate conjunction to empty > -- > > Key: CALCITE-2730 > URL: https://issues.apache.org/jira/browse/CALCITE-2730 > Project: Calcite > Issue Type: Bug > Components: core >Affects Versions: next >Reporter: Stamatis Zampetakis >Assignee: Julian Hyde >Priority: Major > Fix For: 1.18.0 > > > Regression from 1.17. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2730) RelBuilder simplifies a filter with duplicate conjunction to empty
[ https://issues.apache.org/jira/browse/CALCITE-2730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16715075#comment-16715075 ] Julian Hyde commented on CALCITE-2730: -- We’d lose a lot if we just remove duplicates. We should be removing all conditions implied by earlier conditions. > RelBuilder simplifies a filter with duplicate conjunction to empty > -- > > Key: CALCITE-2730 > URL: https://issues.apache.org/jira/browse/CALCITE-2730 > Project: Calcite > Issue Type: Bug > Components: core >Affects Versions: next >Reporter: Stamatis Zampetakis >Assignee: Julian Hyde >Priority: Major > Fix For: 1.18.0 > > > Regression from 1.17. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2730) RelBuilder simplifies a filter with duplicate conjunction to empty
[ https://issues.apache.org/jira/browse/CALCITE-2730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16715036#comment-16715036 ] Zoltan Haindrich commented on CALCITE-2730: --- I think at that point replacing only the actual term would be right - if that ends up triggering some issues; making the input terms unique will probably be an alternate option. > RelBuilder simplifies a filter with duplicate conjunction to empty > -- > > Key: CALCITE-2730 > URL: https://issues.apache.org/jira/browse/CALCITE-2730 > Project: Calcite > Issue Type: Bug > Components: core >Affects Versions: next >Reporter: Stamatis Zampetakis >Assignee: Julian Hyde >Priority: Major > Fix For: 1.18.0 > > > Regression from 1.17. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2730) RelBuilder simplifies a filter with duplicate conjunction to empty
[ https://issues.apache.org/jira/browse/CALCITE-2730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16715020#comment-16715020 ] Stamatis Zampetakis commented on CALCITE-2730: -- OK, now I see better. Probably it is the line [1873|https://github.com/apache/calcite/blob/f3655e15a11a9fb266af290cb390e690d4301c09/core/src/main/java/org/apache/calcite/rex/RexSimplify.java#L1873] which causes the problem after the fix of equals. So the question is should we really replace all terms or just the term in question. I am looking a bit more. > RelBuilder simplifies a filter with duplicate conjunction to empty > -- > > Key: CALCITE-2730 > URL: https://issues.apache.org/jira/browse/CALCITE-2730 > Project: Calcite > Issue Type: Bug > Components: core >Affects Versions: next >Reporter: Stamatis Zampetakis >Assignee: Julian Hyde >Priority: Major > Fix For: 1.18.0 > > > Regression from 1.17. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2730) RelBuilder simplifies a filter with duplicate conjunction to empty
[ https://issues.apache.org/jira/browse/CALCITE-2730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16714965#comment-16714965 ] Zoltan Haindrich commented on CALCITE-2730: --- It was working in 1.17 because "equals" was not working properly ; hence it have only removed 1 from the 2 instances; because of compare by pointer address > RelBuilder simplifies a filter with duplicate conjunction to empty > -- > > Key: CALCITE-2730 > URL: https://issues.apache.org/jira/browse/CALCITE-2730 > Project: Calcite > Issue Type: Bug > Components: core >Affects Versions: next >Reporter: Stamatis Zampetakis >Assignee: Julian Hyde >Priority: Major > Fix For: 1.18.0 > > > Regression from 1.17. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2730) RelBuilder simplifies a filter with duplicate conjunction to empty
[ https://issues.apache.org/jira/browse/CALCITE-2730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16714949#comment-16714949 ] Stamatis Zampetakis commented on CALCITE-2730: -- Naive question, why it was working in 1.17? Who was responsible for the simplification before that is not doing it now? In any case, I plan to look a bit into it now but I am asking just in case you already know the answer. > RelBuilder simplifies a filter with duplicate conjunction to empty > -- > > Key: CALCITE-2730 > URL: https://issues.apache.org/jira/browse/CALCITE-2730 > Project: Calcite > Issue Type: Bug > Components: core >Affects Versions: next >Reporter: Stamatis Zampetakis >Assignee: Julian Hyde >Priority: Major > Fix For: 1.18.0 > > > Regression from 1.17. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2730) RelBuilder simplifies a filter with duplicate conjunction to empty
[
https://issues.apache.org/jira/browse/CALCITE-2730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16714874#comment-16714874
]
Zoltan Haindrich commented on CALCITE-2730:
---
[~zabetak] the issue is with the fact that in the presence of {{x>0}} the other
{{x>0}} is redundant. The issue happens in
[processRange|https://github.com/apache/calcite/blob/f3655e15a11a9fb266af290cb390e690d4301c09/core/src/main/java/org/apache/calcite/rex/RexSimplify.java#L1676]
- this part has it's own issues...for now I think we might be fine if we
would ensure that AND/OR operands are free of duplicates (I think that would
not have any correctness issue).
> RelBuilder simplifies a filter with duplicate conjunction to empty
> --
>
> Key: CALCITE-2730
> URL: https://issues.apache.org/jira/browse/CALCITE-2730
> Project: Calcite
> Issue Type: Bug
> Components: core
>Affects Versions: next
>Reporter: Stamatis Zampetakis
>Assignee: Julian Hyde
>Priority: Major
> Fix For: 1.18.0
>
>
> Regression from 1.17.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2730) RelBuilder simplifies a filter with duplicate conjunction to empty
[ https://issues.apache.org/jira/browse/CALCITE-2730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16713355#comment-16713355 ] Stamatis Zampetakis commented on CALCITE-2730: -- I will try to have a look on Monday. Thanks for the tip [~kgyrtkirk]! > RelBuilder simplifies a filter with duplicate conjunction to empty > -- > > Key: CALCITE-2730 > URL: https://issues.apache.org/jira/browse/CALCITE-2730 > Project: Calcite > Issue Type: Bug > Components: core >Affects Versions: next >Reporter: Stamatis Zampetakis >Assignee: Julian Hyde >Priority: Major > Fix For: 1.18.0 > > > Regression from 1.17. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2730) RelBuilder simplifies a filter with duplicate conjunction to empty
[ https://issues.apache.org/jira/browse/CALCITE-2730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16713095#comment-16713095 ] Zoltan Haindrich commented on CALCITE-2730: --- This seems to be caused by CALCITE-2632; if I "break" the equals functionality it starts behaving correctly; I believe this bug was hiding in the bushes :) > RelBuilder simplifies a filter with duplicate conjunction to empty > -- > > Key: CALCITE-2730 > URL: https://issues.apache.org/jira/browse/CALCITE-2730 > Project: Calcite > Issue Type: Bug > Components: core >Affects Versions: next >Reporter: Stamatis Zampetakis >Assignee: Julian Hyde >Priority: Major > > Regression from 1.17. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2730) RelBuilder simplifies a filter with duplicate conjunction to empty
[ https://issues.apache.org/jira/browse/CALCITE-2730?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16713048#comment-16713048 ] Stamatis Zampetakis commented on CALCITE-2730: -- I added a PR with a simple unit test. The same tests passes with 1.17. I was not yet able to identify which commit caused the regression. > RelBuilder simplifies a filter with duplicate conjunction to empty > -- > > Key: CALCITE-2730 > URL: https://issues.apache.org/jira/browse/CALCITE-2730 > Project: Calcite > Issue Type: Bug > Components: core >Affects Versions: next >Reporter: Stamatis Zampetakis >Assignee: Julian Hyde >Priority: Major > > Regression from 1.17. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
