I think it is ~40 in our suite. Not sure on yours. If you have a failed run on your side, share the output and I may be able to propose a patch to your suite:
It is a one line change. https://github.com/apache/drill/commit/edea8b1cf4e5476d803e8b87c79e08e8c3263e04#diff-ca259849558f34142f1e17066df42a9fR259 Are you guys convinced that this isn't ever a correctness issue? That would be my main hesitation to removing this. It is clearly a bug. -- Jacques Nadeau CTO and Co-Founder, Dremio On Fri, Mar 4, 2016 at 10:35 AM, Parth Chandra <par...@apache.org> wrote: > I'd be more comfortable if we merged this in after the release. Updating > the test baselines will delay the release considerably - I would want the > new baselines to be verified manually which is always time consuming. > How many tests are affected? > > > > On Fri, Mar 4, 2016 at 10:03 AM, Jacques Nadeau <jacq...@dremio.com> > wrote: > > > Do you think we should back out? It seemed like this could likely cause > > correctness issues although we may be safe with our name based > resolution. > > On Mar 4, 2016 9:56 AM, "Aman Sinha" <asi...@maprtech.com> wrote: > > > > > @jacques, thanks for the heads-up, although it comes too close to the > > > release date :). I agree that the plan tests should be targeted to a > > > narrow scope by specifying the sub-pattern it is supposed to test. > That > > > said, it is a lot easier for the tester to capture the entire plan > since > > > he/she may miss an important detail if a sub-plan is captured, so this > > > requires close interaction with the developer (which depending on > various > > > factors may take longer while the test needs to be checked-in). > > > BTW, Calcite unit tests capture entire plan. I am not sure if similar > > > issue has been discussed on Calcite dev list in the past. > > > > > > -Aman > > > > > > On Fri, Mar 4, 2016 at 4:19 AM, Jacques Nadeau <jacq...@dremio.com> > > wrote: > > > > > > > I just merged a simple fix that Laurent found for DRILL-4467. > > > > > > > > This fix ensures consistent column ordering when pushing projection > > into > > > a > > > > scan and invalid plans. This is good and was causing excessive > > operators > > > > and pushdown failure in some cases. > > > > > > > > However, this fix removes a number of trivial projects (that were > > > > previously not detected as such) in a large set of queries. This > means > > > that > > > > a number of plan baselines will need to be updated in the extended > > > > regression suite to avoid consideration of the trivial project. This > > > > underscores an issue I see in these tests. In virtually all cases > I've > > > > seen, the purpose of the test shouldn't care whether the trivial > > project > > > is > > > > part of the plan. However, the baseline is over-reaching in its > > > definition, > > > > including a bunch of nodes irrelevant to the purpose of the test. One > > > > example might be here: > > > > > > > > > > > > > > > > > > https://github.com/mapr/drill-test-framework/blob/master/framework/resources/Functional/filter/pushdown/plan/q23.res > > > > > > > > In this baseline, we're testing that the filter is pushed past the > > > > aggregation. That means what we really need to be testing is a > > multiline > > > > plan pattern of > > > > > > > > HashAgg.*Filter.*Scan.* > > > > > > > > or better > > > > > > > > HashAgg.*Filter\(condition=\[=\(\$0, 10\)\]\).*Scan.* > > > > > > > > However, you can see that the actual expected result includes the > > > > entire structure of the plan (but not the pushed down filter > > > > condition). This causes the plan to fail now that DRILL-4467 is > > > > merged. As part of the fixes to these plans, we should really make > > > > sure that the scope of the baseline is only focused on the relevant > > > > issue to avoid nominal changes from causing testing false positives. > > > > > > > > > > > > > > > > -- > > > > Jacques Nadeau > > > > CTO and Co-Founder, Dremio > > > > > > > > > >
