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