drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Parth Chandra <par...@apache.org>
Subject Re: Heads up on trivial project fix: plan baselines in regression suite need updating
Date Fri, 04 Mar 2016 18:35:35 GMT
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 <jacques@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" <asinha@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 <jacques@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
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message