You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by Vladimir Sitnikov <si...@gmail.com> on 2020/11/02 07:46:02 UTC
Search/Sarg: untested feature merged to the default branch
Hey Julian, thank you for rolling search/sarg expression. I believe it is a
valuable feature.
Unfortunately, the new feature causes noticeable regressions, and it looks
like the regressions are still there :(
I believe the major cause of regressions is missing tests for the added
feature :(
Note: the tests were there before Sarg, however, when you add a new
RexNode, you need to teach RexInterpreter to deal with it.
Currently, RexProgramFuzzyTest ignores Sarg expressions, and it could have
prevented the regressions.
You should have missed my ask to add tests (see
https://github.com/apache/calcite/pull/2233#issuecomment-720113993 ), so
would you please re-add the test coverage (RexInterpreter, RexAnalyzer,
RexProgramFuzzyTest) for Sarg and fix the issues?
Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Vladimir Sitnikov <si...@gmail.com>.
Julian>And I would love a SQL test case that shows that SEARCH is broken. :)
I give you 100 char SQL reproducer and you restore pull/2250.
Deal?
Julian>But it is good news for people using Calcite 1.26 - it is very
Julian>unlikely that they are being affected by the bug.
Calcite 1.26 is affected by sarg-induced
[CALCITE-4325] RexSimplify incorrectly simplifies complex expressions that
contain Sarg and IS NULL
[CALCITE-4352] RelBuilder/RexSimplify/Sarg incorrectly transforms complex
expressions with IS NULL/IS NOT NULL
Both have trivial SQL reproducers behind the lines of (deptno = 20 OR
deptno IS NULL) AND deptno = 10,
so please don't misguide people that "1.26 is fine".
Release 1.26 has severe issues (including SQL-reproducible ones), and many
trivial queries like "... or ... is null" might produce wrong results.
The above-listed issues are in no way an exhaustive list of RexNode-related
issues, so 1.26 is nowhere near for production use in case the system
touches RexNode simplification.
Juilan>This is frustrating for me trying to reproduce a bug before I fix it
Juilan>But it is good news for people using Calcite 1.26 - it is very
Juilan>unlikely that they are being affected by the bug
Of course, it might be complicated to find the bug manually.
That does not imply the bug is not there. Neither it implies the bug is
rare.
It just means you are using the wrong tools and approaches to find the bug.
Julian>CALCITE-4377 <https://issues.apache.org/jira/browse/CALCITE-4377> exists
to fix tech debt introduced in
4377 is a regression, not a tech debt.
Tech debt makes maintenance harder, and tech debt does not result in the
system producing wrong results.
4377 is to fix the bug introduced in 3457.
Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Julian Hyde <jh...@apache.org>.
Pushing release 1.27 back to the New Year makes sense.
We need to do some work on Sargs. As to what that work should be,
things are going to get controversial. (I would appreciate help from
impartial elders like Stamatis, Francis, Haisheng to guide this
discussion to where we can agree.)
I agree with Vladimir that Search does not handle unknown (i.e.
boolean null) values properly. He proposes that Search should always
return true or false, and that is probably right. (That would imply
that we would have to add logic to check for null values before
calling Search.)
But then we run into differences of approach. Vladimir sees this
through the lens of RexFuzzyTest, and wants to get this on a solid
formal basis. (If you look at what Vladimir had to do in
https://github.com/apache/calcite/pull/2250 you will see that in
addition to tech debt in RexFuzzyTest, RexSimplify and elsewhere; bugs
https://issues.apache.org/jira/browse/CALCITE-4399,
https://issues.apache.org/jira/browse/CALCITE-4398,
https://issues.apache.org/jira/browse/CALCITE-4397,
https://issues.apache.org/jira/browse/CALCITE-4388; also, Danny's
https://issues.apache.org/jira/browse/CALCITE-4377 exists to fix tech
debt introduced in
https://issues.apache.org/jira/browse/CALCITE-3457.)
Meanwhile, I see the world through the lens of SQL statements giving
wrong results. A few issues in this area have been logged, and Danny
and I have fixed them (see
https://issues.apache.org/jira/browse/CALCITE-4325,
https://issues.apache.org/jira/browse/CALCITE-4352,
https://issues.apache.org/jira/browse/CALCITE-4364).
I have tried to find a case where SEARCH gives wrong results in the
presence of NULLs, and I have failed. For example, I crafted the
following test case in operator.iq:
diff --git a/core/src/test/resources/sql/operator.iq
b/core/src/test/resources/sql/operator.iq
index 904f1008e..6b71ca553 100644
--- a/core/src/test/resources/sql/operator.iq
+++ b/core/src/test/resources/sql/operator.iq
@@ -66,6 +66,35 @@ select * from "scott".emp where not sal > 1300 and
not sal < 1200;
!ok
+# Sargs and three-valued boolean logic
+select empno, ename, job, comm,
+ comm between 1000 and 2000 as b,
+ case
+ when (comm between 1000 and 2000) is unknown then 'u'
+ when (comm between 1000 and 2000) is true then 't'
+ when (comm between 1000 and 2000) is false then 'f'
+ else '?'
+ end as truth,
+ comm in (0, 100, 200, 300) as b2,
+ comm in (0, 100, 200, 300, null) as b3
+from "scott".emp
+where deptno = 30
+order by empno;
++-------+--------+----------+---------+-------+-------+-------+------+
+| EMPNO | ENAME | JOB | COMM | B | TRUTH | B2 | B3 |
++-------+--------+----------+---------+-------+-------+-------+------+
+| 7499 | ALLEN | SALESMAN | 300.00 | false | f | true | true |
+| 7521 | WARD | SALESMAN | 500.00 | false | f | false | |
+| 7654 | MARTIN | SALESMAN | 1400.00 | true | t | false | |
+| 7698 | BLAKE | MANAGER | | | u | | |
+| 7844 | TURNER | SALESMAN | 0.00 | false | f | true | true |
+| 7900 | JAMES | CLERK | | | u | | |
++-------+--------+----------+---------+-------+-------+-------+------+
+(6 rows)
+
+!ok
+
+
# MULTISET EXCEPT
values multiset ['a', 'c', 'a'] multiset except multiset ['a'];
+--------+
By putting the condition in the SELECT clause, we see TRUE, FALSE and
UNKNOWN as distinct values; conditions are usually used in WHERE,
HAVING or ON, where UNKNOWN and FALSE are both treated as FALSE. Note
that the 'b' column has a mix of true, false and blank (unknown)
values.
Knowing that Search doesn't handle null values, I expected the query
to fail, but Calcite gives the correct results (I checked them on
hsqldb). I surmise that when Search is converted back to operators
during code generation, we also ignore 3-valued logic, and so the two
bugs cancel out.
This is frustrating for me trying to reproduce a bug before I fix it.
But it is good news for people using Calcite 1.26 - it is very
unlikely that they are being affected by the bug.
Back to our priorities before 1.27. I think we should continue to
identify and burn down tech debt in RexSimplify, RexProgramFuzzyTest.
That includes revisiting issues raised by Vladimir, above. (I am
skeptical about a few of these, such as that RexSimplify should not
change the nullability of an expression.)
To summarize. We need a measured discussion about what is wrong (level
of impact, level of effort to fix, and degree of disruption if we fix
it, or revert it) and I would appreciate the help of the elders to
moderate that discussion.
And I would love a SQL test case that shows that SEARCH is broken. :)
Julian
On Mon, Nov 16, 2020 at 1:56 PM Francis Chuang <fr...@apache.org> wrote:
>
> We currently maintain a release cadence of a new release every two
> months. This puts the next release pretty close to Christmas next month.
>
> Would it make sense if we push 1.27.0 back a few weeks until maybe after
> new year? In the mean time, perhaps we can focus on getting these issues
> fixed and improve the reliability of search/sarge? Perhaps even to the
> extent where we divert some resources from other features and put more
> focus into search/sarge.
>
> Francis
>
> On 17/11/2020 5:38 am, Julian Hyde wrote:
> > And yet more passive-aggressive bulls**t from Vladimir:
> >
> > https://github.com/apache/calcite/commit/c6b3fb7ddbf8aed981ff3fb0632c77216dbe68d6
> >
> > I've force-pushed to remove it.
> >
> > I plan to respond to Stamatis' thoughtful email later today. Also, I
> > think I may have a test case that demonstrates the issues that
> > Vladimir is talking about; if so, I'll log a JIRA case. But for now, I
> > need to get on with my work week.
> >
> > Julian
> >
> >
> >
> >
> > On Mon, Nov 16, 2020 at 3:28 AM Vladimir Sitnikov
> > <si...@gmail.com> wrote:
> >>
> >> Stamatis>@Vladimir: Can you specify which JIRA issue(s) you would like to
> >> see solved
> >> Stamatis>for the next release?
> >>
> >> RexProgramFuzzyTest should be enabled, and Sarg-related fuzzing should be
> >> implemented as well.
> >> RexFuzzer should generate Sarg expressions.
> >> RexInterpreter should be able to evaluate search/sarg.
> >> RexProgramFuzzyTest should pass both with and without expandSearch.
> >>
> >> I'm afraid I have no time on executing RexProgramFuzzyTest and logging
> >> failures as JIRA cases.
> >> I assume committers should ensure the code they commit passes existing
> >> tests, and they should ensure new code is accompanied by tests as well.
> >>
> >> It looks like I tried everything:
> >> a) I kindly asked Julian to refrain from committing untested code (both in
> >> PR and in the very first message of the thread)
> >> b) I highlighted major design issues (see unknownAs in [1])
> >> c) I provided sample failures (see "sample input" in [2])
> >> d) I even suggested the plan to resolve the issues: "revert search/sarg",
> >> "re-enable fuzzer", "fix issues discovered by the fuzzer", "re-add
> >> search/sarg".
> >> Not only I suggested the approach, I implemented a noticeable part of it
> >> (e.g.I re-enabled the fuzzer and added relevant code fixes to make the test
> >> pass, see CALCITE-4398, CALCITE-4397, CALCITE-4388), however, Julian
> >> discarded all of it with "A commit war is no way to proceed" comment.
> >>
> >>
> >> Now I stop doing everything related to search/sarg.
> >> Of course, if there's a release, and the fuzzer is still disabled, I would
> >> put my -1 vote.
> >>
> >> [1]
> >> https://lists.apache.org/thread.html/r602b7e17cf076f00dfbd946e814cea098a7b81e0b252bfa6e2fd5e9b%40%3Cdev.calcite.apache.org%3E
> >> [2]
> >> https://lists.apache.org/thread.html/r6d509f7202195362b33f93c076b5010bd1a9a5ac1e4fa6814819077c%40%3Cdev.calcite.apache.org%3E
> >>
> >> Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Francis Chuang <fr...@apache.org>.
We currently maintain a release cadence of a new release every two
months. This puts the next release pretty close to Christmas next month.
Would it make sense if we push 1.27.0 back a few weeks until maybe after
new year? In the mean time, perhaps we can focus on getting these issues
fixed and improve the reliability of search/sarge? Perhaps even to the
extent where we divert some resources from other features and put more
focus into search/sarge.
Francis
On 17/11/2020 5:38 am, Julian Hyde wrote:
> And yet more passive-aggressive bulls**t from Vladimir:
>
> https://github.com/apache/calcite/commit/c6b3fb7ddbf8aed981ff3fb0632c77216dbe68d6
>
> I've force-pushed to remove it.
>
> I plan to respond to Stamatis' thoughtful email later today. Also, I
> think I may have a test case that demonstrates the issues that
> Vladimir is talking about; if so, I'll log a JIRA case. But for now, I
> need to get on with my work week.
>
> Julian
>
>
>
>
> On Mon, Nov 16, 2020 at 3:28 AM Vladimir Sitnikov
> <si...@gmail.com> wrote:
>>
>> Stamatis>@Vladimir: Can you specify which JIRA issue(s) you would like to
>> see solved
>> Stamatis>for the next release?
>>
>> RexProgramFuzzyTest should be enabled, and Sarg-related fuzzing should be
>> implemented as well.
>> RexFuzzer should generate Sarg expressions.
>> RexInterpreter should be able to evaluate search/sarg.
>> RexProgramFuzzyTest should pass both with and without expandSearch.
>>
>> I'm afraid I have no time on executing RexProgramFuzzyTest and logging
>> failures as JIRA cases.
>> I assume committers should ensure the code they commit passes existing
>> tests, and they should ensure new code is accompanied by tests as well.
>>
>> It looks like I tried everything:
>> a) I kindly asked Julian to refrain from committing untested code (both in
>> PR and in the very first message of the thread)
>> b) I highlighted major design issues (see unknownAs in [1])
>> c) I provided sample failures (see "sample input" in [2])
>> d) I even suggested the plan to resolve the issues: "revert search/sarg",
>> "re-enable fuzzer", "fix issues discovered by the fuzzer", "re-add
>> search/sarg".
>> Not only I suggested the approach, I implemented a noticeable part of it
>> (e.g.I re-enabled the fuzzer and added relevant code fixes to make the test
>> pass, see CALCITE-4398, CALCITE-4397, CALCITE-4388), however, Julian
>> discarded all of it with "A commit war is no way to proceed" comment.
>>
>>
>> Now I stop doing everything related to search/sarg.
>> Of course, if there's a release, and the fuzzer is still disabled, I would
>> put my -1 vote.
>>
>> [1]
>> https://lists.apache.org/thread.html/r602b7e17cf076f00dfbd946e814cea098a7b81e0b252bfa6e2fd5e9b%40%3Cdev.calcite.apache.org%3E
>> [2]
>> https://lists.apache.org/thread.html/r6d509f7202195362b33f93c076b5010bd1a9a5ac1e4fa6814819077c%40%3Cdev.calcite.apache.org%3E
>>
>> Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Julian Hyde <jh...@apache.org>.
And yet more passive-aggressive bulls**t from Vladimir:
https://github.com/apache/calcite/commit/c6b3fb7ddbf8aed981ff3fb0632c77216dbe68d6
I've force-pushed to remove it.
I plan to respond to Stamatis' thoughtful email later today. Also, I
think I may have a test case that demonstrates the issues that
Vladimir is talking about; if so, I'll log a JIRA case. But for now, I
need to get on with my work week.
Julian
On Mon, Nov 16, 2020 at 3:28 AM Vladimir Sitnikov
<si...@gmail.com> wrote:
>
> Stamatis>@Vladimir: Can you specify which JIRA issue(s) you would like to
> see solved
> Stamatis>for the next release?
>
> RexProgramFuzzyTest should be enabled, and Sarg-related fuzzing should be
> implemented as well.
> RexFuzzer should generate Sarg expressions.
> RexInterpreter should be able to evaluate search/sarg.
> RexProgramFuzzyTest should pass both with and without expandSearch.
>
> I'm afraid I have no time on executing RexProgramFuzzyTest and logging
> failures as JIRA cases.
> I assume committers should ensure the code they commit passes existing
> tests, and they should ensure new code is accompanied by tests as well.
>
> It looks like I tried everything:
> a) I kindly asked Julian to refrain from committing untested code (both in
> PR and in the very first message of the thread)
> b) I highlighted major design issues (see unknownAs in [1])
> c) I provided sample failures (see "sample input" in [2])
> d) I even suggested the plan to resolve the issues: "revert search/sarg",
> "re-enable fuzzer", "fix issues discovered by the fuzzer", "re-add
> search/sarg".
> Not only I suggested the approach, I implemented a noticeable part of it
> (e.g.I re-enabled the fuzzer and added relevant code fixes to make the test
> pass, see CALCITE-4398, CALCITE-4397, CALCITE-4388), however, Julian
> discarded all of it with "A commit war is no way to proceed" comment.
>
>
> Now I stop doing everything related to search/sarg.
> Of course, if there's a release, and the fuzzer is still disabled, I would
> put my -1 vote.
>
> [1]
> https://lists.apache.org/thread.html/r602b7e17cf076f00dfbd946e814cea098a7b81e0b252bfa6e2fd5e9b%40%3Cdev.calcite.apache.org%3E
> [2]
> https://lists.apache.org/thread.html/r6d509f7202195362b33f93c076b5010bd1a9a5ac1e4fa6814819077c%40%3Cdev.calcite.apache.org%3E
>
> Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Vladimir Sitnikov <si...@gmail.com>.
Stamatis>@Vladimir: Can you specify which JIRA issue(s) you would like to
see solved
Stamatis>for the next release?
RexProgramFuzzyTest should be enabled, and Sarg-related fuzzing should be
implemented as well.
RexFuzzer should generate Sarg expressions.
RexInterpreter should be able to evaluate search/sarg.
RexProgramFuzzyTest should pass both with and without expandSearch.
I'm afraid I have no time on executing RexProgramFuzzyTest and logging
failures as JIRA cases.
I assume committers should ensure the code they commit passes existing
tests, and they should ensure new code is accompanied by tests as well.
It looks like I tried everything:
a) I kindly asked Julian to refrain from committing untested code (both in
PR and in the very first message of the thread)
b) I highlighted major design issues (see unknownAs in [1])
c) I provided sample failures (see "sample input" in [2])
d) I even suggested the plan to resolve the issues: "revert search/sarg",
"re-enable fuzzer", "fix issues discovered by the fuzzer", "re-add
search/sarg".
Not only I suggested the approach, I implemented a noticeable part of it
(e.g.I re-enabled the fuzzer and added relevant code fixes to make the test
pass, see CALCITE-4398, CALCITE-4397, CALCITE-4388), however, Julian
discarded all of it with "A commit war is no way to proceed" comment.
Now I stop doing everything related to search/sarg.
Of course, if there's a release, and the fuzzer is still disabled, I would
put my -1 vote.
[1]
https://lists.apache.org/thread.html/r602b7e17cf076f00dfbd946e814cea098a7b81e0b252bfa6e2fd5e9b%40%3Cdev.calcite.apache.org%3E
[2]
https://lists.apache.org/thread.html/r6d509f7202195362b33f93c076b5010bd1a9a5ac1e4fa6814819077c%40%3Cdev.calcite.apache.org%3E
Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Enrico Olivelli <eo...@gmail.com>.
Stamatis,
Il Lun 16 Nov 2020, 10:20 Stamatis Zampetakis <za...@gmail.com> ha
scritto:
> Hello,
>
> I don't want the current situation to be disheartening for anybody,
> especially those who spend significant time on these code changes (Julian,
> Vladimir).
> Our common goal is to release 1.27.0 in the best possible conditions so
> let's try to agree on the plan to move forward.
> To do this, I think it is important to answer the following questions:
>
> @Vladimir: Can you specify which JIRA issue(s) you would like to see solved
> for the next release?
> @all (committers or not): Did you postpone the upgrade to Calcite 1.26.0
> due to the problems found by Vladimir?
>
In HerdDB we still haven't updated Calcite because of Sarg, but only
because the change requires few changes and we still do not need to upgrade
yet, so the upgrade is only in our backlog.
Given Vladimir's precious findings we will wait for 1.27.
> As soon as we have the JIRA ids:
> @all (committers or not): Do we have somebody willing to work on the issues
> found by Vladimir?
>
I am sorry I don't have cycles
Enrico
> Best,
> Stamatis
>
> On Sat, Nov 14, 2020 at 4:55 PM Julian Hyde <jh...@gmail.com>
> wrote:
>
> > Vladimir,
> >
> > I’ve reverted your revert. A commit war is no way to proceed.
> >
> > Julian
> >
> >
> > > On Nov 14, 2020, at 2:55 AM, Vladimir Sitnikov <
> > sitnikov.vladimir@gmail.com> wrote:
> > >
> > > I've reverted SEARCH. Sorry for the inconvenience.
> > >
> > > Here's the PR that re-adds SEARCH:
> > > https://github.com/apache/calcite/pull/2263
> > > Please feel free to pick it up. I expect certain commits should be
> > squashed
> > > (e.g. search should be re-added as a single commit).
> > >
> > > I'm not sure I would have time to make it happen.
> > >
> > > Frankly speaking, I would suggest we should make SEARCH operator never
> > > return null.
> > > In other words, SEARCH(X, [Y, Z]) should be the same as "(X is not
> > distinct
> > > from Y) or (X is not distinct from Z)".
> > > The old semantics was like SEARCH(null, [42]) => unknown, SEARCH(null,
> > > [null, 42]) => false which results in wrong results in simplification.
> > >
> > > "Expand search" might want to receive unknownAs parameter.
> > >
> > > Vladimir
> >
> >
>
Re: Search/Sarg: untested feature merged to the default branch
Posted by Stamatis Zampetakis <za...@gmail.com>.
Hello,
I don't want the current situation to be disheartening for anybody,
especially those who spend significant time on these code changes (Julian,
Vladimir).
Our common goal is to release 1.27.0 in the best possible conditions so
let's try to agree on the plan to move forward.
To do this, I think it is important to answer the following questions:
@Vladimir: Can you specify which JIRA issue(s) you would like to see solved
for the next release?
@all (committers or not): Did you postpone the upgrade to Calcite 1.26.0
due to the problems found by Vladimir?
As soon as we have the JIRA ids:
@all (committers or not): Do we have somebody willing to work on the issues
found by Vladimir?
Best,
Stamatis
On Sat, Nov 14, 2020 at 4:55 PM Julian Hyde <jh...@gmail.com> wrote:
> Vladimir,
>
> I’ve reverted your revert. A commit war is no way to proceed.
>
> Julian
>
>
> > On Nov 14, 2020, at 2:55 AM, Vladimir Sitnikov <
> sitnikov.vladimir@gmail.com> wrote:
> >
> > I've reverted SEARCH. Sorry for the inconvenience.
> >
> > Here's the PR that re-adds SEARCH:
> > https://github.com/apache/calcite/pull/2263
> > Please feel free to pick it up. I expect certain commits should be
> squashed
> > (e.g. search should be re-added as a single commit).
> >
> > I'm not sure I would have time to make it happen.
> >
> > Frankly speaking, I would suggest we should make SEARCH operator never
> > return null.
> > In other words, SEARCH(X, [Y, Z]) should be the same as "(X is not
> distinct
> > from Y) or (X is not distinct from Z)".
> > The old semantics was like SEARCH(null, [42]) => unknown, SEARCH(null,
> > [null, 42]) => false which results in wrong results in simplification.
> >
> > "Expand search" might want to receive unknownAs parameter.
> >
> > Vladimir
>
>
Re: Search/Sarg: untested feature merged to the default branch
Posted by Julian Hyde <jh...@gmail.com>.
Vladimir,
I’ve reverted your revert. A commit war is no way to proceed.
Julian
> On Nov 14, 2020, at 2:55 AM, Vladimir Sitnikov <si...@gmail.com> wrote:
>
> I've reverted SEARCH. Sorry for the inconvenience.
>
> Here's the PR that re-adds SEARCH:
> https://github.com/apache/calcite/pull/2263
> Please feel free to pick it up. I expect certain commits should be squashed
> (e.g. search should be re-added as a single commit).
>
> I'm not sure I would have time to make it happen.
>
> Frankly speaking, I would suggest we should make SEARCH operator never
> return null.
> In other words, SEARCH(X, [Y, Z]) should be the same as "(X is not distinct
> from Y) or (X is not distinct from Z)".
> The old semantics was like SEARCH(null, [42]) => unknown, SEARCH(null,
> [null, 42]) => false which results in wrong results in simplification.
>
> "Expand search" might want to receive unknownAs parameter.
>
> Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Vladimir Sitnikov <si...@gmail.com>.
I've reverted SEARCH. Sorry for the inconvenience.
Here's the PR that re-adds SEARCH:
https://github.com/apache/calcite/pull/2263
Please feel free to pick it up. I expect certain commits should be squashed
(e.g. search should be re-added as a single commit).
I'm not sure I would have time to make it happen.
Frankly speaking, I would suggest we should make SEARCH operator never
return null.
In other words, SEARCH(X, [Y, Z]) should be the same as "(X is not distinct
from Y) or (X is not distinct from Z)".
The old semantics was like SEARCH(null, [42]) => unknown, SEARCH(null,
[null, 42]) => false which results in wrong results in simplification.
"Expand search" might want to receive unknownAs parameter.
Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Vladimir Sitnikov <si...@gmail.com>.
Julian>Removing it at this point would be disruptive
The sooner the revert the less the disruption is.
Julian>Let’s log bugs so that they can make an informed decision
It would be awesome if somebody picks up the task.
I've spent the past weekend resolving the issue thanks to the SEARCH PR
which was merged despite the test failures.
That is not fun.
Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Julian Hyde <jh...@gmail.com>.
I have not spoken up because others have expressed my thoughts. Namely:
1. While there are problems with Search/Sarg it is a net benefit. Removing it at this point would be disruptive.
2. Everyone except Vladimir opposes committing Vladimir’s PR to revert the change.
3. It would be helpful if Vladimir logs JIRA cases for the outstanding issues with Search/Sarg.
Let’s not get tied up in legal semantics about whether a change that reverts another change is itself a change and therefore be vetoed. We’d be setting ourselves up for a commit/revert war. Instead, let’s try and reach consensus about how to make 1.27 as good as possible.
It’s not a perfect solution, but people who do not want Search have the option to stay on release 1.25. Let’s log bugs so that they can make an informed decision.
Julian
> On Nov 13, 2020, at 12:28 PM, Vladimir Sitnikov <si...@gmail.com> wrote:
>
> Ruben>if they are not solved by then, then revert
>
> The revert would be hard if we delay it :(
> For instance, CALCITE-4383 and CALCITE-4389 do not seem to be related to
> SEARCH, however, they touch the same files.
>
> Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Vladimir Sitnikov <si...@gmail.com>.
Ruben>if they are not solved by then, then revert
The revert would be hard if we delay it :(
For instance, CALCITE-4383 and CALCITE-4389 do not seem to be related to
SEARCH, however, they touch the same files.
Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Ruben Q L <ru...@gmail.com>.
I share the concerns raised by Stamatis and Haisheng.
I understand the motivations behind Vladimir's decision, I would not oppose
a revert but I'm not 100% sure at this point it is the best approach.
I would lean towards creating bug tickets in Jira describing the known
issues, with version "1.27" and priority "blocker", work on them and when
the release date approaches re-evaluate the situation (if they are not
solved by then, then revert).
Best regards,
Ruben
On Fri, Nov 13, 2020 at 6:03 PM Vladimir Sitnikov <
sitnikov.vladimir@gmail.com> wrote:
> I see there is a desire to keep SEARCH in the next release.
> I see there's a silence as well (e.g. some committers/PMCs are not
> replying) which is understandable.
>
> However, there's even more: Danny and Julian commit more changes/features
> to search/sarg feature.
> I guess it was unintentional, however, Julian committed CALCITE-4383 and
> CALCITE-4389. The commits depend on SEARCH code, so I can't really revert
> SEARCH and keep 4383/4389 at the same time (e.g. the new changes
> modify SqlInternalOperators which was added in "Add internal SEARCH
> operator" commit)
>
> So I would have to revert some of the recent commits as well :(
>
> Vladimir
>
Re: Search/Sarg: untested feature merged to the default branch
Posted by Vladimir Sitnikov <si...@gmail.com>.
I see there is a desire to keep SEARCH in the next release.
I see there's a silence as well (e.g. some committers/PMCs are not
replying) which is understandable.
However, there's even more: Danny and Julian commit more changes/features
to search/sarg feature.
I guess it was unintentional, however, Julian committed CALCITE-4383 and
CALCITE-4389. The commits depend on SEARCH code, so I can't really revert
SEARCH and keep 4383/4389 at the same time (e.g. the new changes
modify SqlInternalOperators which was added in "Add internal SEARCH
operator" commit)
So I would have to revert some of the recent commits as well :(
Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Vladimir Sitnikov <si...@gmail.com>.
Danny>We all vote -1 and what's the problem there you still want to revert ?
Danny, I see only you mention "-1", however, I see no technical reasons, so
I assume the veto has no weight.
I see the general feeling is like "oh, we want to keep the SEARCH feature
in the subsequent release" (which is my feeling as well!),
however, I see nobody commits to fixing SEARCH issues.
Note: the codebase has issues that are caused by SEARCH, and it has issues
that existed **before** SEARCH.
It makes it harder to fix SEARCH since it requires to fix all the bugs at
the same time.
Now I step in, I fix RexSimplify old issues, I log bugs for further
discussion (like CALCITE-4397).
It is really sad to hear "you always push the things that rejected by most
of the fellows".
Danny>If the RexProgramFuzzyTest can not run, just fix it
Let me please repeat: the issue is that SEARCH produces invalid results,
and Sarg produces NPEs.
Sample input: case_(eq(vInt(1), vInt(1)), falseLiteral, lt(vInt(1),
literal(1)))
Error is result mismatch: when applied to {?0.int1=NULL}, CASE(=(?0.int1,
?0.int1), false, <(?0.int1, 1)) yielded NULL, and false yielded false
Danny>it is the parallelism test that cause the Sarg state unstable.
The above sample can be run in isolation, and it does result in wrong
results.
It might be a nice idea to double-check if SEARCH implementation has a
shared mutable state.
---
Of course, the fuzzer might uncover more issues if it generates Sarg
expressions as well (it should not be hard to implement by the way),
however, I would stress that the current Sarg produces wrong results even
for regular (no SEARCH) inputs which impacts all Calcite users.
Believe me, I want to get SEARCH working as well, however, it might take
significant efforts, so I believe it would be more efficient
if we revert the feature, fix tests, add workarounds to make tests pass
again, and then try re-adding SEARCH.
As you probably know, CALCITE-3457 introduced a regression that is present
for more than a year now.
Note that the regression was identified right after the merge, and nobody
cared for the whole year.
That allowed bad code to creep unnoticed, and now we have several
RexSimplify issues that are not really related to SEARCH.
Note: my "revert search" pull request fixes CALCITE-3457 regression, so I
strongly disagree with your wording of "reverting the code brainless".
I truly believe the revert moves Calcite code forward, and it enables us to
re-integrate SEARCH in a better and nicer way.
Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Danny Chan <da...@apache.org>.
Stop the merge, PLEASE !!!
We all vote -1 and what's the problem there you still want to revert ?
What's the bug/problem there that need to be fixed? Can you log some issues
there ?
If the RexProgramFuzzyTest can not run, just fix it. From my local test, it
is not caused by the Sarg itself, it is the parallelism test that cause the
Sarg state unstable.
Why you always push the things that rejected by most of the fellows, i
don't understand.
Vladimir Sitnikov <si...@gmail.com> 于2020年11月13日周五 上午3:09写道：
> Ok, it took me even more time to stabilize the build than I initially
> expected as there were 4 misbehaving issues (4388, 4397, 4398, 4399)
>
> The PR that reverts SEARCH is https://github.com/apache/calcite/pull/2250
> I'll let the dust settle, and I merge it.
>
> Hopefully, SEARCH then could be re-added when it is ready.
>
> Vladimir
>
Re: Search/Sarg: untested feature merged to the default branch
Posted by Vladimir Sitnikov <si...@gmail.com>.
Ok, it took me even more time to stabilize the build than I initially
expected as there were 4 misbehaving issues (4388, 4397, 4398, 4399)
The PR that reverts SEARCH is https://github.com/apache/calcite/pull/2250
I'll let the dust settle, and I merge it.
Hopefully, SEARCH then could be re-added when it is ready.
Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Haisheng Yuan <hy...@apache.org>.
I have the same feeling with Stamatis.
I completely understand that Vladimir is trying to keep it working correctly. However, I know some downstream project already updated Calcite version and changed their code to adapt to the Sarg/SEARCH operator. Reverting it and in case we failed to fix the regressions you mentioned, if we release the new version without getting it back, there will be another major breaking change again. If that is the case, I would rather push for the fixes instead of revert before we can release next version.
Thanks,
Haisheng Yuan
On 2020/11/09 08:32:20, Vladimir Sitnikov <si...@gmail.com> wrote:
> Danny>-1 for the revert, we should fix the issues we encountered instead of
> Danny>reverting the code brainless for a whole release.
>
> Hi Danny, did you just veto the code change? (see
> https://www.apache.org/foundation/voting.html#Veto )
> I am afraid I fail to find a technical justification behind the veto.
>
> Danny>So, logger the fail cases with issues and fix them, that is the way
> to go.
>
> Thanks for the suggestion. I would tentatively decline it as it's going to
> be difficult to find time to do this.
>
> Vladimir
>
Re: Search/Sarg: untested feature merged to the default branch
Posted by Vladimir Sitnikov <si...@gmail.com>.
Danny>-1 for the revert, we should fix the issues we encountered instead of
Danny>reverting the code brainless for a whole release.
Hi Danny, did you just veto the code change? (see
https://www.apache.org/foundation/voting.html#Veto )
I am afraid I fail to find a technical justification behind the veto.
Danny>So, logger the fail cases with issues and fix them, that is the way
to go.
Thanks for the suggestion. I would tentatively decline it as it's going to
be difficult to find time to do this.
Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Danny Chan <da...@apache.org>.
-1 for the revert, we should fix the issues we encountered instead of
reverting the code brainless for a whole release.
At lease, project like Apache Flink has upgrade to version 1.26 and the
Sarg feature overall looks good.
We are trying to fix the Sarg issues in version 1.27 and we should continue
with that.
So, logger the fail cases with issues and fix them, that is the way to go.
Vladimir Sitnikov <si...@gmail.com> 于2020年11月9日周一 上午7:41写道：
> Stamatis>People who are responsible for upgrading Calcite, tend to follow
> the dev
> Stamatis>list so they can take the necessary actions.
>
> I think behind the lines of updating https://calcite.apache.org/news/
>
> We might want to mention the following regressions, and we might want to
> mark the release as unstable/broken
> since there's no workaround:
> [CALCITE-4352] RexSimplify incorrectly drops IS NULL and IS NOT NULL from
> SEARCH expressions
> [CALCITE-4325] RexSimplify incorrectly simplifies complex expressions that
> contain Sarg and IS NULL
> [CALCITE-4173], fix assertion error when RexSimplify generates Sarg with
> single null only
>
> >Reverting the feature for the next release will necessitate further
> actions
> >on their side and it may be counter productive.
>
> Reverting the feature != reverting the feature for the next release.
> It might be the feature will be ready by the next release.
>
> I just don't want to keep the code and tests broken.
> There are lots of contributions landing to RexSimplify (which is nice, and
> I love optimizations),
> however, we have disabled RexProgramFuzzyTest long ago, and we really need
> to re-enable it
> otherwise, we would keep introducing regressions.
>
> Vladimir
>
Re: Search/Sarg: untested feature merged to the default branch
Posted by Vladimir Sitnikov <si...@gmail.com>.
Stamatis>People who are responsible for upgrading Calcite, tend to follow
the dev
Stamatis>list so they can take the necessary actions.
I think behind the lines of updating https://calcite.apache.org/news/
We might want to mention the following regressions, and we might want to
mark the release as unstable/broken
since there's no workaround:
[CALCITE-4352] RexSimplify incorrectly drops IS NULL and IS NOT NULL from
SEARCH expressions
[CALCITE-4325] RexSimplify incorrectly simplifies complex expressions that
contain Sarg and IS NULL
[CALCITE-4173], fix assertion error when RexSimplify generates Sarg with
single null only
>Reverting the feature for the next release will necessitate further actions
>on their side and it may be counter productive.
Reverting the feature != reverting the feature for the next release.
It might be the feature will be ready by the next release.
I just don't want to keep the code and tests broken.
There are lots of contributions landing to RexSimplify (which is nice, and
I love optimizations),
however, we have disabled RexProgramFuzzyTest long ago, and we really need
to re-enable it
otherwise, we would keep introducing regressions.
Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Stamatis Zampetakis <za...@gmail.com>.
Thanks for raising awareness Vladimir.
To be honest, I am not sure how we should move forward but I think it's a
bit late to ask people not to upgrade.
We can let people know about the potential issues but I guess this is
already done via this mail.
People who are responsible for upgrading Calcite, tend to follow the dev
list so they can take the necessary actions.
I assume that there are already people who upgraded to 1.26 and they spend
some time and effort to make it happen.
Reverting the feature for the next release will necessitate further actions
on their side and it may be counter productive.
In any case, I think it is important to create the appropriate JIRAs with
the queries that fail.
That way we can have a better view of what needs to be fixed for the next
release and track the progress.
If we decide to go for a broader announcement then the JIRAs will also be
useful as a reference.
Best,
Stamatis
On Sat, Nov 7, 2020 at 11:37 PM Vladimir Sitnikov <
sitnikov.vladimir@gmail.com> wrote:
> I've created https://github.com/apache/calcite/pull/2250 to check if
> revert
> helps at all.
>
> Vladimir
>
Re: Search/Sarg: untested feature merged to the default branch
Posted by Vladimir Sitnikov <si...@gmail.com>.
I've created https://github.com/apache/calcite/pull/2250 to check if revert
helps at all.
Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Vladimir Sitnikov <si...@gmail.com>.
Everybody,
I suggest we revert Sarg, and merge it back when it is ready (passes tests).
If no objections, I would proceed with reverting Sarg in 72 hours.
I believe we should announce that Calcite 1.26 has severe issues with
SEARCH,
and people should refrain from using the version. They might corrupt their
data, which would be very sad.
The current design and implementation for SEARCH has a major issue with
nullness
For instance, nullableInt=4 is known to return unknown for null input.
However, search(x, [..]) is ill-defined, especially when multiple search
expressions are combined.
simplify(unknownAsTrue, nullableInt=4), simplify(unknownAsUnknown,
nullableInt=4), simplify(unknownAsFalse, nullableInt=4)
are not easily represented in the current SEARCH format, so it causes wrong
results.
---
Even though I like Sarg idea, it looks like the current implementation is
not really ready, and causes "wrong results from optimization".
I did try approaching the issues, and I believe, the best we should do is
to revert the thing completely,
and reiterate adding it.
Frankly speaking, I think we should incorporate "unknownAsTrue /
unknownAsUnknown" into search/sarg (instead of the current "containsNull"),
however, it would take time.
An alternative option is to keep the code in the repository and hope for
someone to fix issues.
However, I would vote -1 (veto) for the release if nobody fixes issues. The
justification is the feature basically blocks
Calcite usage, and it can't be disabled.
I do not have bad feelings about "untested code sitting in Git".
The issues for me are:
1) I won't be able to upgrade to the newer Calcite
2) The current search/sarg implementation has issues with Java null
handling. In other words, @nullable annotations
do not easily fit there, and the code is going to change a lot since the
are functional issues.
It is extremely annoying to shoot the moving target while you know the
target does not work.
3) I would have to revert mat-calcite-plugin to the previous Calcite
version (and fix source-incompatible changes if any)
Vladimir
Re: Search/Sarg: untested feature merged to the default branch
Posted by Julian Hyde <jh...@apache.org>.
It's going to be difficult to find time to do this. I would appreciate
some help.
Julian
On Sun, Nov 1, 2020 at 11:46 PM Vladimir Sitnikov
<si...@gmail.com> wrote:
>
> Hey Julian, thank you for rolling search/sarg expression. I believe it is a
> valuable feature.
>
> Unfortunately, the new feature causes noticeable regressions, and it looks
> like the regressions are still there :(
>
> I believe the major cause of regressions is missing tests for the added
> feature :(
> Note: the tests were there before Sarg, however, when you add a new
> RexNode, you need to teach RexInterpreter to deal with it.
>
> Currently, RexProgramFuzzyTest ignores Sarg expressions, and it could have
> prevented the regressions.
>
> You should have missed my ask to add tests (see
> https://github.com/apache/calcite/pull/2233#issuecomment-720113993 ), so
> would you please re-add the test coverage (RexInterpreter, RexAnalyzer,
> RexProgramFuzzyTest) for Sarg and fix the issues?
>
> Vladimir