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 21:50:35 GMT
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
View raw message