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