trafodion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Eric Owhadi <eric.owh...@esgyn.com>
Subject RE: question on ValueIdSet::isNotNullable
Date Wed, 06 Jan 2016 19:01:24 GMT
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
View raw message