trafodion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hans Zeller <hans.zel...@esgyn.com>
Subject Re: question on ValueIdSet::isNotNullable
Date Wed, 06 Jan 2016 21:56:20 GMT
I'll be happy to submit this - once I got some problems in my testing
environment straightened out. I assume you can remove your changes to
ValueIdSet::isNotNullable() without waiting for me, since it would just
cause a temporary performance problem?

Thanks,

Hans

On Wed, Jan 6, 2016 at 1:52 PM, Dave Birdsall <dave.birdsall@esgyn.com>
wrote:

> For what it is worth, Hans change looks good. So if he submits a pull
> request to Trafodion, I think it can be merged quickly (after tests
> complete
> of course).
>
> -----Original Message-----
> From: Eric Owhadi [mailto:eric.owhadi@esgyn.com]
> Sent: Wednesday, January 6, 2016 1:51 PM
> To: dev@trafodion.incubator.apache.org
> Subject: RE: question on ValueIdSet::isNotNullable
>
> It would be easy for me to add the fix but I am afraid my checkin is
> already
> so big, I don't want to make it bigger. So I would prefer a JIRA, but not
> working on it until my code is in. You will see in my new code something
> similar where AND is accounted for like in previous predicate push down,
> however, this is legitimate, as mine go deep down recursive and it is to
> handle staff like a>0 OR (b=0 and c>0) that would end up parsed
> recursively.
> I will however revert my change and implement your, have you checked it in
> master? Or should I just add it in as part of my checkin?
> Eric
>
>
> -----Original Message-----
> From: Hans Zeller [mailto:hans.zeller@esgyn.com]
> Sent: Wednesday, January 6, 2016 3:38 PM
> To: dev <dev@trafodion.incubator.apache.org>
> Subject: Re: question on ValueIdSet::isNotNullable
>
> Hi again,
>
> In the meantime I was able to reproduce and debug this. The bug is in
> method
> ValueIdSet::replaceVEGExpressions(). That method iterates over the members
> and replaces VEGRefs and VEGPreds one by one. It also replaces RangeSpec
> predicates with the original predicate, which could be an AND.
>
> In this case, we have a RangeSpec(a in [10, 20]), and the method replaces
> that with the original predicate (a >= 10 AND a <= 20). This AND should
> inserted into the result as a set of individual predicates, but this method
> fails to do that and inserts the AND instead. Here is a patch:
>
> diff --git a/core/sql/optimizer/ValueDesc.cpp
> b/core/sql/optimizer/ValueDesc.cpp
> index 372da1e..590eca9 100644
> --- a/core/sql/optimizer/ValueDesc.cpp
> +++ b/core/sql/optimizer/ValueDesc.cpp
> @@ -3165,7 +3165,11 @@ void ValueIdSet::replaceVEGExpressions
>            if (iePtr != exprId.getItemExpr())  // a replacement was done
>             {
>               subtractElement(exprId);        // remove existing ValueId
> -             newExpr += iePtr->getValueId(); // replace with a new one
> +              // insert new expression(s)
> +              if (iePtr->getOperatorType() == ITM_AND)
> +                iePtr->convertToValueIdSet(newExpr, NULL, ITM_AND,
> + FALSE,
> FALSE);
> +              else
> +                newExpr += iePtr->getValueId();
>             }
>         }
>        else // delete the ValueId of the VEGPredicate/VEGReference from the
> set
>
>
> We should also remove the code that handles ANDs in method
> HbaseAccess::extractHbaseFilterPreds(). That probably was added because of
> the same bug.
>
> I can file a JIRA and fix it, or you can fix it as part of your checkin.
> Let me know which way you would prefer.
>
> Hans
>
> On Wed, Jan 6, 2016 at 11:01 AM, Eric Owhadi <eric.owhadi@esgyn.com>
> wrote:
>
> > I found this using:
> > Select * from t where a between 10 and 20
> >
> > I am guessing that the between transform into and happens after the
> > code that is flattening ANDs?
> >
> > But again, given I found it during L1 and given the code will just
> > result in sub optimal optimization, if the same issue is happening on
> > other construct I would not know...
> > Eric
> >
> > -----Original Message-----
> > From: Hans Zeller [mailto:hans.zeller@esgyn.com]
> > Sent: Wednesday, January 6, 2016 11:24 AM
> > To: dev <dev@trafodion.incubator.apache.org>
> > Subject: Re: question on ValueIdSet::isNotNullable
> >
> > Hi Eric, this is a really basic assumption what a ValueIdSet for
> > predicates means, and there should never be an AND operator in a
> > ValueIdSet. Therefore we should definitely not add code that deals
> > with an AND and you should not be afraid to fix this, it will only
> > make the code better.
> >
> > Can you point me to the code where this happens, I want to make sure
> > that I don't misunderstand the context.
> >
> > Thanks,
> >
> > Hans
> >
> > On Wed, Jan 6, 2016 at 6:40 AM, Eric Owhadi <eric.owhadi@esgyn.com>
> wrote:
> >
> > > Hi Qifan
> > > Nice simplification. I didn't go that path originally seeing that it
> > > would redundantly repeat check on supportSQLNullLogical, and create
> > > heap activity for single element LIST handling when none is needed.
> > > However, seeing how compact the resulting code is and because deep
> > > AND predicates are unlikely (not like OR that can go very deep with
> > > IN statement), the code you propose is a better approach, I'll
> > > change to it thanks.
> > >
> > > Key column can support one NULL (I have been told). So I cannot just
> > > assume that because a column is part of a primary key, it is
> > > therefore not nullable.
> > > Key column is retrieved for a single purpose: make sure that if the
> > > useful column retrieved is nullable and the value retrieved can be
> > > null, then at least one non nullable column is retrieved along with
> > > the usefull one, to make sure "absent" null from the usefull column
> > > are detected (since we will still retrieve an element for each row
> > > on the non nullable column added).
> > >
> > > Eric
> > >
> > > -----Original Message-----
> > > From: Qifan Chen [mailto:qifan.chen@esgyn.com]
> > > Sent: Tuesday, January 5, 2016 11:07 PM
> > > To: dev <dev@trafodion.incubator.apache.org>
> > > Subject: Re: question on ValueIdSet::isNotNullable
> > >
> > > Hi,
> > >
> > > One comment.
> > >
> > > I sensed some redundancy between the newly added help function and
> > > the existing ValueIdSet::isNotNullable().
> > >
> > > Maybe the new recursive help function is not needed and instead to
> > > code the new AND case as follows.
> > >
> > > case ITM_AND:
> > >    {
> > >       ValueIdSet x;
> > >       x.insert(iePtr->child(0)->getValueId());
> > >       x.insert(iePtr->child(1)->getValueId());
> > >       return x.isNotNullable(opd);
> > >    }
> > >
> > > And one question.  Maybe the key column is retrieved for other
> > > purpose as a key column never satisfies NULLs by default.
> > >
> > > Thanks --Qifan
> > >
> > > On Tue, Jan 5, 2016 at 10:05 PM, Eric Owhadi <eric.owhadi@esgyn.com>
> > > wrote:
> > >
> > > > HI Hans, unfortunately,  that is not the case. Even the predicate
> > > > push down version 1 code take care of both cases. And I have seen
> > > > both cases too in my various testing. I have not tried to single
> > > > out why that is and why one is picked vs the other... just assumed
> > > > that I needed to account for both.
> > > > Cheers,
> > > > Eric
> > > > On Jan 5, 2016 9:50 PM, "Hans Zeller" <hans.zeller@esgyn.com> wrote:
> > > >
> > > > > Hi Eric, I think the reason this method does not consider AND is
> > > > > that a ValueIdSet should not contain any AND predicates, since
> > > > > the set itself assumes an ANDing of all of its members. So,
> > > > > instead of a single member AND(an>=20, an<=30), the ValueIdSet
> > > > > should contain two
> > > > > members: an>=20, an<=30, in which case the method would return
> TRUE.
> > > > >
> > > > > Hans
> > > > >
> > > > > On Tue, Jan 5, 2016 at 5:39 PM, Eric Owhadi
> > > > > <eric.owhadi@esgyn.com>
> > > > wrote:
> > > > >
> > > > > > Hi Trafodioneers,
> > > > > >
> > > > > > Not sure if the author of this isNotNullable function is still
> > > > > > active,
> > > > > but
> > > > > > since this is the only part in the code I am about to submit
> > > > > > that will
> > > > > not
> > > > > > be cqd protected, I would like to ask if I am exposing
> > > > > > trafodion to
> > > > some
> > > > > > side effect. Basically running L1 testing, I discovered that
> > > > > > doing:
> > > > > >
> > > > > > Select an from t140 where an between 20 and 30
> > > > > >
> > > > > > Would not optimize the column returned, and incorrectly add
a
> > > > > > key
> > > > column
> > > > > to
> > > > > > satisfy NULLs (an being a nullable column).
> > > > > >
> > > > > > However, given the predicate an cannot be null, therefore
> > > > > > there should
> > > > > not
> > > > > > be any additional column retrieved.
> > > > > >
> > > > > >
> > > > > >
> > > > > > The issue is in the original  NABoolean
> > > > > > ValueIdSet::isNotNullable(const ValueId& opd)
> > > > > >
> > > > > >
> > > > > >
> > > > > > It is assuming that predicates are a bunch of ANDed together
> > > > > > stuff in
> > > > the
> > > > > > ValueIdSet.
> > > > > >
> > > > > > However, in my case, the resulting stuff is a single valueId
> > > > > > in the valueIdSet, that has as first operator and ITM_AND.
> > > > > >
> > > > > > So I believe the correct implementation taking care of both
> > > > > > cases of
> > > > AND
> > > > > > should be the one bellow. Does that make sense?
> > > > > >
> > > > > >
> > > > > >
> > > > > > Or should I CQD protect this to only be the behavior with the
> > > > > > predicate push down V2. I don’t think so, but cannot be 100%
> > > > > > certain…
> > > > > >
> > > > > > The original code is simply the same removing the line between
> > > > > > ~~EO
> > > > begin
> > > > > > and ~~EO end.
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > //~~EO begin
> > > > > >
> > > > > > //helper function for isNotNullable recusivity on AND
> > > > > >
> > > > > > NABoolean isNotNullableRecurse(const ValueId& vid, const
> > > > > > ValueId&
> > > > > > opd)
> > > > > >
> > > > > > {
> > > > > >
> > > > > >                 ItemExpr *iePtr = vid.getItemExpr();
> > > > > >
> > > > > >                 switch (iePtr->getOperatorType()) {
> > > > > >
> > > > > >                     case ITM_AND:
> > > > > >
> > > > > >                                 return
> > > > > >
> > > > isNotNullableRecurse(iePtr->child(0)->castToItemExpr()->getValueId
> > > > ()
> > > > ,o
> > > > pd)
> > > > > > ||
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > isNotNullableRecurse(iePtr->child(1)->castToItemExpr()->getValueId
> > > > ()
> > > > ,o
> > > > pd);
> > > > > >
> > > > > >                                 break;
> > > > > >
> > > > > >                     case ITM_IS_NOT_NULL:
> > > > > >
> > > > > >                       if
> > > > > > (iePtr->child(0)->containsTheGivenValue(
> > > > > > opd
> > > > ))
> > > > > {
> > > > > >
> > > > > >                         // found "opd IS NOT NULL" predicate
> > > > > > conjunct
> > > > > >
> > > > > >                         return TRUE; // opd is not null
> > > > > >
> > > > > >                       }
> > > > > >
> > > > > >                       break; // keep looking
> > > > > >
> > > > > >                     case ITM_EQUAL:
> > > > > >
> > > > > >                     case ITM_NOT_EQUAL:
> > > > > >
> > > > > >                     case ITM_LESS:
> > > > > >
> > > > > >                     case ITM_LESS_EQ:
> > > > > >
> > > > > >                     case ITM_GREATER:
> > > > > >
> > > > > >                     case ITM_GREATER_EQ:
> > > > > >
> > > > > >                       if
> > > > > > (iePtr->child(0)->containsTheGivenValue(
> > > > > > opd )
> > > > > ||
> > > > > >
> > > > > >
> > > > > > iePtr->child(1)->containsTheGivenValue(
> > > > > > opd
> > > > ))
> > > > > {
> > > > > >
> > > > > >                         // found "opd compop expr" predicate
> > > > > > conjunct
> > > > > >
> > > > > >                         return TRUE; // opd is not null
> > > > > >
> > > > > >                       }
> > > > > >
> > > > > >                       break; // keep looking
> > > > > >
> > > > > >                     case ITM_IS_NULL:
> > > > > >
> > > > > >                       if
> > > > > > (iePtr->child(0)->containsTheGivenValue(
> > > > > > opd
> > > > ))
> > > > > {
> > > > > >
> > > > > >                         // found "opd IS NULL" predicate
> > > > > > conjunct
> > > > > >
> > > > > >                         return FALSE; // opd is null
> > > > > >
> > > > > >                       }
> > > > > >
> > > > > >                       break;
> > > > > >
> > > > > >                     default:
> > > > > >
> > > > > >                       break; // keep looking
> > > > > >
> > > > > >
> > > > > >
> > > > > >                   }
> > > > > >
> > > > > >                   return FALSE;
> > > > > >
> > > > > > }
> > > > > >
> > > > > > //~~EO end
> > > > > >
> > > > > > //
> > > > > > --------------------------------------------------------------
> > > > > > --
> > > > > > --
> > > > > > --
> > > > > >
> > > > > > // return true iff ValueIdSet has predicates that guarantee
> > > > > >
> > > > > > // that opd is not nullable
> > > > > >
> > > > > > //
> > > > > > --------------------------------------------------------------
> > > > > > --
> > > > > > --
> > > > > > --
> > > > > >
> > > > > > NABoolean ValueIdSet::isNotNullable(const ValueId& opd)
> > > > > >
> > > > > > {
> > > > > >
> > > > > >   // if opd's type is not nullable then return TRUE
> > > > > >
> > > > > >   if (!opd.getType().supportsSQLnullLogical()) {
> > > > > >
> > > > > >     return TRUE;
> > > > > >
> > > > > >   }
> > > > > >
> > > > > >   // opd's type is nullable.
> > > > > >
> > > > > >
> > > > > >
> > > > > >   // search ValueIdSet for a predicate conjunct
> > > > > >
> > > > > >   // that can guarantee that opd is not null
> > > > > >
> > > > > >   for (ValueId vid = init(); next(vid); advance(vid) ) {
> > > > > >
> > > > > >     // predicates are implicitly connected by AND
> > > > > >
> > > > > >     ItemExpr *iePtr = vid.getItemExpr();
> > > > > >
> > > > > >     switch (iePtr->getOperatorType()) {
> > > > > >
> > > > > > //~~EO begin
> > > > > >
> > > > > >     case ITM_AND:
> > > > > >
> > > > > >                 return
> > > > > >
> > > > isNotNullableRecurse(iePtr->child(0)->castToItemExpr()->getValueId
> > > > ()
> > > > ,o
> > > > pd)
> > > > > > ||
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > isNotNullableRecurse(iePtr->child(1)->castToItemExpr()->getValueId
> > > > ()
> > > > ,o
> > > > pd);
> > > > > >
> > > > > >                 break;
> > > > > >
> > > > > > //~~EO end
> > > > > >
> > > > > >     case ITM_IS_NOT_NULL:
> > > > > >
> > > > > >       if (iePtr->child(0)->containsTheGivenValue( opd
)) {
> > > > > >
> > > > > >         // found "opd IS NOT NULL" predicate conjunct
> > > > > >
> > > > > >         return TRUE; // opd is not null
> > > > > >
> > > > > >       }
> > > > > >
> > > > > >       break; // keep looking
> > > > > >
> > > > > >     case ITM_EQUAL:
> > > > > >
> > > > > >     case ITM_NOT_EQUAL:
> > > > > >
> > > > > >     case ITM_LESS:
> > > > > >
> > > > > >     case ITM_LESS_EQ:
> > > > > >
> > > > > >     case ITM_GREATER:
> > > > > >
> > > > > >     case ITM_GREATER_EQ:
> > > > > >
> > > > > >       if (iePtr->child(0)->containsTheGivenValue( opd
) ||
> > > > > >
> > > > > >           iePtr->child(1)->containsTheGivenValue( opd
)) {
> > > > > >
> > > > > >         // found "opd compop expr" predicate conjunct
> > > > > >
> > > > > >         return TRUE; // opd is not null
> > > > > >
> > > > > >       }
> > > > > >
> > > > > >       break; // keep looking
> > > > > >
> > > > > >     case ITM_IS_NULL:
> > > > > >
> > > > > >       if (iePtr->child(0)->containsTheGivenValue( opd
)) {
> > > > > >
> > > > > >         // found "opd IS NULL" predicate conjunct
> > > > > >
> > > > > >         return FALSE; // opd is null
> > > > > >
> > > > > >       }
> > > > > >
> > > > > >       break;
> > > > > >
> > > > > >     default:
> > > > > >
> > > > > >       break; // keep looking
> > > > > >
> > > > > >     }
> > > > > >
> > > > > >   }
> > > > > >
> > > > > >   return FALSE;
> > > > > >
> > > > > > }
> > > > > >
> > > > >
> > > >
> > >
> > >
> > >
> > > --
> > > Regards, --Qifan
> > >
> >
>

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