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