You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by Enrico Olivelli <eo...@gmail.com> on 2018/09/21 16:06:07 UTC
[DISCUSS] Problem with 'assert' ignored in production, leading to
inconsistent query results
Hi,
one of my customer got into a very weird situation when a merge join
was returning less results than expected.
Reproducing the issue on a unit test the query execution resulted in
an assertion error so that is was easy to find the problem (a bug in
the top level application in this case).
This is the case
https://github.com/apache/calcite/blob/56def3946d7e30706b2845464f7fc575c66e4694/linq4j/src/main/java/org/apache/calcite/linq4j/EnumerableDefaults.java#L3257
This is the code in MergeJoinEnumerator:
if (c != 0) {
assert c < 0 : "not sorted";
break;
}
I am reaching dev list because I would like to raise the discussion
about this kind of assertions, which in my opinion should be turned
into explicit invariant checks to be executed even in production code.
IMHO it is better to have something that fails instead of something
which return non accurate results silently (SQL is not an approximate
language).
It there is agreement I will scan Calcite codebase and look for
potential "assert" to be turned into explicit checks which will throw
IllegalStateException (or other runtime exception).
Thoughts ?
Enrico
Re: [DISCUSS] Problem with 'assert' ignored in production, leading to
inconsistent query results
Posted by Enrico Olivelli <eo...@gmail.com>.
Il giorno ven 21 set 2018 alle ore 20:03 Enrico Olivelli
<eo...@gmail.com> ha scritto:
>
>
>
> Il ven 21 set 2018, 19:45 Julian Hyde <jh...@apache.org> ha scritto:
>>
>> Well, maybe we can.
>>
>> But look at the premise of the question: "Problem with 'assert'
>> ignored in production, leading to inconsistent query results". The
>> problem is not with the assert.
>
>
> Yes actually the bug is not clear.
>
> We have a merge join with 2 unsorted inputs. I don't know if the problem is in the planner (why it had choosen a merge join and not a simple joon ) or in the translation from planner output to actual plan
>
> I will be back with news next week.
> But I will start a new thread.
>
> Thank you for you comments and suggestions.
> I will evaluate if removing the assert with introduce too much cost (comparing large strings for instance)
This is the a patch, the assertion is an "integer < 0" check so it is
very cheap in Java, I think it is worth
https://issues.apache.org/jira/browse/CALCITE-2591
I have started in another thread the discussion about the actual bug
which led to such situation
Thank you
Enrico
>
> Enrico
>
>> On Fri, Sep 21, 2018 at 10:23 AM Vladimir Sitnikov
>> <si...@gmail.com> wrote:
>> >
>> > Julian>We can't afford to check assumptions in performance-critical code.
>> >
>> > We definitely can afford checks that take a couple of CPU cycles to perform.
>> >
>> > Vladimir
>
> --
>
>
> -- Enrico Olivelli
Re: [DISCUSS] Problem with 'assert' ignored in production, leading to
inconsistent query results
Posted by Enrico Olivelli <eo...@gmail.com>.
Il ven 21 set 2018, 19:45 Julian Hyde <jh...@apache.org> ha scritto:
> Well, maybe we can.
>
> But look at the premise of the question: "Problem with 'assert'
> ignored in production, leading to inconsistent query results". The
> problem is not with the assert.
>
Yes actually the bug is not clear.
We have a merge join with 2 unsorted inputs. I don't know if the problem is
in the planner (why it had choosen a merge join and not a simple joon ) or
in the translation from planner output to actual plan
I will be back with news next week.
But I will start a new thread.
Thank you for you comments and suggestions.
I will evaluate if removing the assert with introduce too much cost
(comparing large strings for instance)
Enrico
On Fri, Sep 21, 2018 at 10:23 AM Vladimir Sitnikov
> <si...@gmail.com> wrote:
> >
> > Julian>We can't afford to check assumptions in performance-critical code.
> >
> > We definitely can afford checks that take a couple of CPU cycles to
> perform.
> >
> > Vladimir
>
--
-- Enrico Olivelli
Re: [DISCUSS] Problem with 'assert' ignored in production, leading to
inconsistent query results
Posted by Julian Hyde <jh...@apache.org>.
Well, maybe we can.
But look at the premise of the question: "Problem with 'assert'
ignored in production, leading to inconsistent query results". The
problem is not with the assert.
On Fri, Sep 21, 2018 at 10:23 AM Vladimir Sitnikov
<si...@gmail.com> wrote:
>
> Julian>We can't afford to check assumptions in performance-critical code.
>
> We definitely can afford checks that take a couple of CPU cycles to perform.
>
> Vladimir
Re: [DISCUSS] Problem with 'assert' ignored in production, leading to
inconsistent query results
Posted by Vladimir Sitnikov <si...@gmail.com>.
Julian>We can't afford to check assumptions in performance-critical code.
We definitely can afford checks that take a couple of CPU cycles to perform.
Vladimir
Re: [DISCUSS] Problem with 'assert' ignored in production, leading to
inconsistent query results
Posted by Julian Hyde <jh...@apache.org>.
If you have a problem with code behaving differently in production,
let's take out the assert.
You (or we) screwed up. The input promised that it was sorted, but it
wasn't. We can't afford to check assumptions in performance-critical
code.
I'm sorry that it took a long time to debug. But don't blame the messenger.
Julian
On Fri, Sep 21, 2018 at 9:14 AM Vladimir Sitnikov
<si...@gmail.com> wrote:
>
> +1 for converting that assert to exception with the relevant text.
>
> https://issues.apache.org/jira/browse/CALCITE-1890 would probably help
> there to identify the relation in question (Enumerable.toString can be
> added to exception message).
>
> On the other hand, I don't think we should enable expensive checks by
> default.
>
> Vladimir
Re: [DISCUSS] Problem with 'assert' ignored in production, leading to
inconsistent query results
Posted by Vladimir Sitnikov <si...@gmail.com>.
+1 for converting that assert to exception with the relevant text.
https://issues.apache.org/jira/browse/CALCITE-1890 would probably help
there to identify the relation in question (Enumerable.toString can be
added to exception message).
On the other hand, I don't think we should enable expensive checks by
default.
Vladimir