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 <[email protected]> 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 <[email protected]> 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) <[email protected]> > 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) > >> > >
