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) >>
