You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by "Julian Hyde (JIRA)" <ji...@apache.org> on 2014/07/09 03:46:04 UTC
[jira] [Resolved] (OPTIQ-313) Query decorrelation fails
[ https://issues.apache.org/jira/browse/OPTIQ-313?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Julian Hyde resolved OPTIQ-313.
-------------------------------
Resolution: Fixed
Fixed in http://git-wip-us.apache.org/repos/asf/incubator-optiq/commit/58ddf6bf.
> Query decorrelation fails
> -------------------------
>
> Key: OPTIQ-313
> URL: https://issues.apache.org/jira/browse/OPTIQ-313
> Project: Optiq
> Issue Type: Bug
> Reporter: Julian Hyde
> Assignee: Julian Hyde
>
> TPC-H query 2 fails during preparation because even after decorrelation there is a CorrelatorRel present.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
Re: [jira] [Resolved] (OPTIQ-313) Query decorrelation fails
Posted by Julian Hyde <ju...@gmail.com>.
On Jul 9, 2014, at 11:54 AM, Jacques Nadeau <ja...@apache.org> wrote:
> When there is disagreement, we need to work through it.
I agree - and here we are, working through it!
I’m sorry I made a commit after you’d expressed objections. That isn’t allowed in the Apache process, so I won’t do it again.
But I hope it’s clear that I was doing it in good faith. I’d discovered a bug, a test case, and a fix, so I checked it in. One extra test case passing is progress. I suspected that it would break your use of the code, so I started a dialog, trying to find out what exactly your problem was. I figured, heck, you could even use my test case as a starting point for yours.
I know you’d like to have the discussion about test cases in a separate thread, but they really are central to this issue. If I can’t change a piece of code because it breaks some code that I can’t see, it will slow the process down. If I’d not been proactive and had just committed this bug fix, you probably wouldn’t have noticed until a month or two down the line, and then we’d all be scratching our head. But if there had been a working test case, we’d not even be having this discussion. I’d not have made the commit, and in fact, I would not have made the botched merge that probably caused the whole thing in the first place.
Julian
Re: [jira] [Resolved] (OPTIQ-313) Query decorrelation fails
Posted by Jacques Nadeau <ja...@apache.org>.
I think the test case discussion is an important discussion and we should
have it. I suggest we have a separate email thread about that.
The issue I brought up here is a unilateral commit that another committer
had already expressed reservations about. It has a negative impact on the
community. When there is disagreement, we need to work through it.
You can see that happening with OPTIQ-333. I've not committed it even
though that would make my life easier. I recognize that you have
reservations and I want to make sure that the community agrees with the
approach. Once we have consensus, I'll merge it.
I hate email for these discussions because they lose the nuance of a
personal interaction. I'm not worried about this discussion and am happy
working with you. I think this is part of incubation and a step in moving
to a broader community model than how the Optiq community has previously
worked. I'm bringing this up now in the hopes that it will make the
community stronger.
thanks,
Jacques
On Wed, Jul 9, 2014 at 10:42 AM, Julian Hyde <ju...@hydromatic.net> wrote:
> I checked in the code and marked the bug fixed, and was going to explain
> further on the list. I was going to suggest that you log a bug with a test
> case. Please do that. Once I have that test case, It should be easy to fix,
> and I will fix it with some urgency.
>
> I checked in because it was an easy fix to make, it fixed test cases
> including TPC-H Q2, and it didn’t break any existing tests. I have other
> tasks to complete, namely bushy join optimization, and this issue has been
> blocking me for more than a week from getting to that task.
>
> The root of the problem is lack of test cases. The Drill project have
> contributed a lot of patches to Optiq — and I am grateful for this — but
> few of them have been accompanied by test cases. I have noted it but
> haven’t pressed the issue. We are both software engineers, and we both know
> that there is a tradeoff. You save time in the short run, but you are
> vulnerable to changes in Optiq’s functionality. If you are using a piece of
> functionality, don’t just contribute the fix, you need to contribute a test
> case.
>
> This is in part why I am pushing back on
> https://issues.apache.org/jira/browse/OPTIQ-333. You are refactoring
> Optiq so that you can depend on a key internal API, and you have not
> contributed a single test case. If the functionality or API changes and
> Drill breaks, whose problem will it be?
>
> If we are having a broader discussion, it should be whether we as a
> project should accept code changes without test cases. And if we do accept
> such a change, is there any implied contract to keep that functionality
> working?
>
> Julian
>
>
> On Jul 9, 2014, at 10:14 AM, Jacques Nadeau <ja...@apache.org> wrote:
>
> > For these types of changes, I think we need to have a broader discussion
> > before merging commits. We've spent a substantial amount of time in the
> > last six months trying to fix a large number of bugs that were in
> > decorrelation. I'm not surprised that more have been found as it is very
> > complicated code. While this particular change may address this bug, I
> > think it introduces a number of other issues. I'm all for quick fixes
> and
> > minimal process around those. However, if someone is working with code
> > that someone else has put a substantial amount of time into, it would be
> > great to make sure everyone is on the same page before committing.
> >
> > Could you please share a more in-depth analysis to why you think removing
> > this was the right fix? E.g. what the symptoms were and where you saw
> > decorrelation failing with this enabled. We'd be happy to work through a
> > more holistic fix.
> >
> >
> > On Tue, Jul 8, 2014 at 6:46 PM, Julian Hyde (JIRA) <ji...@apache.org>
> wrote:
> >
> >>
> >> [
> >>
> https://issues.apache.org/jira/browse/OPTIQ-313?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
> >> ]
> >>
> >> Julian Hyde resolved OPTIQ-313.
> >> -------------------------------
> >>
> >> Resolution: Fixed
> >>
> >> Fixed in
> >> http://git-wip-us.apache.org/repos/asf/incubator-optiq/commit/58ddf6bf.
> >>
> >>> Query decorrelation fails
> >>> -------------------------
> >>>
> >>> Key: OPTIQ-313
> >>> URL: https://issues.apache.org/jira/browse/OPTIQ-313
> >>> Project: Optiq
> >>> Issue Type: Bug
> >>> Reporter: Julian Hyde
> >>> Assignee: Julian Hyde
> >>>
> >>> TPC-H query 2 fails during preparation because even after decorrelation
> >> there is a CorrelatorRel present.
> >>
> >>
> >>
> >> --
> >> This message was sent by Atlassian JIRA
> >> (v6.2#6252)
> >>
>
>
Re: [jira] [Resolved] (OPTIQ-313) Query decorrelation fails
Posted by Julian Hyde <ju...@hydromatic.net>.
I checked in the code and marked the bug fixed, and was going to explain further on the list. I was going to suggest that you log a bug with a test case. Please do that. Once I have that test case, It should be easy to fix, and I will fix it with some urgency.
I checked in because it was an easy fix to make, it fixed test cases including TPC-H Q2, and it didn’t break any existing tests. I have other tasks to complete, namely bushy join optimization, and this issue has been blocking me for more than a week from getting to that task.
The root of the problem is lack of test cases. The Drill project have contributed a lot of patches to Optiq — and I am grateful for this — but few of them have been accompanied by test cases. I have noted it but haven’t pressed the issue. We are both software engineers, and we both know that there is a tradeoff. You save time in the short run, but you are vulnerable to changes in Optiq’s functionality. If you are using a piece of functionality, don’t just contribute the fix, you need to contribute a test case.
This is in part why I am pushing back on https://issues.apache.org/jira/browse/OPTIQ-333. You are refactoring Optiq so that you can depend on a key internal API, and you have not contributed a single test case. If the functionality or API changes and Drill breaks, whose problem will it be?
If we are having a broader discussion, it should be whether we as a project should accept code changes without test cases. And if we do accept such a change, is there any implied contract to keep that functionality working?
Julian
On Jul 9, 2014, at 10:14 AM, Jacques Nadeau <ja...@apache.org> wrote:
> For these types of changes, I think we need to have a broader discussion
> before merging commits. We've spent a substantial amount of time in the
> last six months trying to fix a large number of bugs that were in
> decorrelation. I'm not surprised that more have been found as it is very
> complicated code. While this particular change may address this bug, I
> think it introduces a number of other issues. I'm all for quick fixes and
> minimal process around those. However, if someone is working with code
> that someone else has put a substantial amount of time into, it would be
> great to make sure everyone is on the same page before committing.
>
> Could you please share a more in-depth analysis to why you think removing
> this was the right fix? E.g. what the symptoms were and where you saw
> decorrelation failing with this enabled. We'd be happy to work through a
> more holistic fix.
>
>
> On Tue, Jul 8, 2014 at 6:46 PM, Julian Hyde (JIRA) <ji...@apache.org> wrote:
>
>>
>> [
>> https://issues.apache.org/jira/browse/OPTIQ-313?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
>> ]
>>
>> Julian Hyde resolved OPTIQ-313.
>> -------------------------------
>>
>> Resolution: Fixed
>>
>> Fixed in
>> http://git-wip-us.apache.org/repos/asf/incubator-optiq/commit/58ddf6bf.
>>
>>> Query decorrelation fails
>>> -------------------------
>>>
>>> Key: OPTIQ-313
>>> URL: https://issues.apache.org/jira/browse/OPTIQ-313
>>> Project: Optiq
>>> Issue Type: Bug
>>> Reporter: Julian Hyde
>>> Assignee: Julian Hyde
>>>
>>> TPC-H query 2 fails during preparation because even after decorrelation
>> there is a CorrelatorRel present.
>>
>>
>>
>> --
>> This message was sent by Atlassian JIRA
>> (v6.2#6252)
>>
Re: [jira] [Resolved] (OPTIQ-313) Query decorrelation fails
Posted by Jacques Nadeau <ja...@apache.org>.
For these types of changes, I think we need to have a broader discussion
before merging commits. We've spent a substantial amount of time in the
last six months trying to fix a large number of bugs that were in
decorrelation. I'm not surprised that more have been found as it is very
complicated code. While this particular change may address this bug, I
think it introduces a number of other issues. I'm all for quick fixes and
minimal process around those. However, if someone is working with code
that someone else has put a substantial amount of time into, it would be
great to make sure everyone is on the same page before committing.
Could you please share a more in-depth analysis to why you think removing
this was the right fix? E.g. what the symptoms were and where you saw
decorrelation failing with this enabled. We'd be happy to work through a
more holistic fix.
On Tue, Jul 8, 2014 at 6:46 PM, Julian Hyde (JIRA) <ji...@apache.org> wrote:
>
> [
> https://issues.apache.org/jira/browse/OPTIQ-313?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
> ]
>
> Julian Hyde resolved OPTIQ-313.
> -------------------------------
>
> Resolution: Fixed
>
> Fixed in
> http://git-wip-us.apache.org/repos/asf/incubator-optiq/commit/58ddf6bf.
>
> > Query decorrelation fails
> > -------------------------
> >
> > Key: OPTIQ-313
> > URL: https://issues.apache.org/jira/browse/OPTIQ-313
> > Project: Optiq
> > Issue Type: Bug
> > Reporter: Julian Hyde
> > Assignee: Julian Hyde
> >
> > TPC-H query 2 fails during preparation because even after decorrelation
> there is a CorrelatorRel present.
>
>
>
> --
> This message was sent by Atlassian JIRA
> (v6.2#6252)
>