Hi Dave, I guess the debating point is about the meaning of "areAllChosenPreds" in method areAllChosenPredsEqualPreds().
Since the subject is a searchKey instance, areAllChosenPreds is well defined and should not be empty (at least my interpretation). I am afraid that if we remove those 4 lines of code, later if the same method is used, we still forget to test the empty-ness of the key which may be important. Thanks --Qifan On Thu, Oct 1, 2015 at 2:31 PM, Qifan Chen <[email protected]> wrote: > Hi Dave, > > You may be right with the mathematician's interpretation :-). > > My thinking of that method is that it tests two things: > 1. existing predicates on keys > 2. all these predicates are equal predicates > > If there is no predicate on keys (which is this case is about), then the > 1st test above fail, and therefore the method should return false. > > Thanks --Qifan > > On Thu, Oct 1, 2015 at 1:44 PM, Dave Birdsall <[email protected]> > wrote: > >> Hi, >> >> Hmmm... I would have thought the logical thing would be: every universally >> quantified predicate is true on an empty set. At least that's the way a >> mathematician or logician would look at it. >> >> I could add the extra checks, but this and one other place (similar code >> in >> an update rule) are the only places where this particular function >> areAllChosenPredsEqualPreds() are called. It would seem simpler to delete >> 4 >> lines of code than to add 2 more tests in 2 places? >> >> Dave >> >> -----Original Message----- >> From: Qifan Chen [mailto:[email protected]] >> Sent: Thursday, October 1, 2015 11:39 AM >> To: dev <[email protected]> >> Subject: Re: Optimizer question >> >> Hi Dave, >> >> From logic point, I think the code below does the right thing, because >> there >> is no predicates present. >> >> >> if ((allChosenPredsAreEqualPreds) && >> (allBeginKeysMissing) && >> (allEndKeysMissing)) >> allChosenPredsAreEqualPreds = FALSE; >> >> >> To fix the problem you found, I wonder if we add some additional checks >> shown below. >> >> if ((! skey->isUnique()) && >> >> (skey->areAllChosenPredsEqualPreds() || >> skey->areAllBeginKeysMissing() || skey->areAllEndKeysMissing()) && >> >> (NOT bef->noCheck()) && >> >> (CmpCommon::getDefault(HBASE_SQL_IUD_SEMANTICS) == DF_ON) && >> >> (CmpCommon::getDefault(HBASE_UPDEL_CURSOR_OPT) == DF_ON) && >> >> ((ActiveSchemaDB()->getDefaults()).getAsLong(COMP_INT_74) == 0)) >> >> result = NULL; >> >> thanks --Qifan >> >> On Thu, Oct 1, 2015 at 1:00 PM, Dave Birdsall <[email protected]> >> wrote: >> >> > Hi, >> > >> > >> > >> > I’m working on code to cost DELETEs in Trafodion. >> > >> > >> > >> > One of the rules in the Optimizer (see ImplRule.cpp) is the >> > HbaseDeleteRule. This rule decides whether to consider a delete plan >> > consisting of just a “trafodion_delete” node. By experimentation, it >> > seems to fire when all of the keys are covered with equality >> > predicates. If only a leading subset of the keys are covered, it does >> > not fire. But if none of the keys are covered by any predicates >> (example: >> > delete from t;), it fires! >> > >> > >> > >> > I can see considering a “trafodion_delete” only plan if all the keys >> > are covered. We are deleting a unique row. This plan uses the Hbase >> > delete API, deleting one row at a time. When there is only one row, >> > this makes perfect sense. >> > >> > >> > >> > The alternative plan is a Scan + a Tuple_flow + a Trafodion_delete or >> > Trafodion_delete_vsbb, the latter uses the Hbase deleteRows API to >> > delete several rows in one shot. The alternative plan is likely more >> > efficient whenever we are deleting multiple rows. >> > >> > >> > >> > So it makes sense not to consider a “trafodion_delete” only plan when >> > not all the keys are covered. But what seems odd is the case where >> > there are no key predicates at all. Why would we consider such a plan >> > then? >> > >> > >> > >> > Drilling down into the code: In HbaseDeleteRule::nextSubstitute is the >> > following: >> > >> > >> > >> > if ((! skey->isUnique()) && >> > >> > (skey->areAllChosenPredsEqualPreds()) && >> > >> > (NOT bef->noCheck()) && >> > >> > (CmpCommon::getDefault(HBASE_SQL_IUD_SEMANTICS) == DF_ON) && >> > >> > (CmpCommon::getDefault(HBASE_UPDEL_CURSOR_OPT) == DF_ON) && >> > >> > ((ActiveSchemaDB()->getDefaults()).getAsLong(COMP_INT_74) == 0)) >> > >> > result = NULL; >> > >> > >> > >> > When this ‘if’ is taken, the “trafodion_delete” only plan is not >> > considered. >> > >> > >> > >> > Here, “skey” is a SearchKey object which represents a begin/end key >> > pair, that is, a subset of rows within the clustering key range to be >> > processed. >> > For the unique row case, “skey->isUnique()” returns true, so the “if” >> > is not taken. So, the “trafodion_delete” only plan IS considered. >> > >> > >> > >> > For the non-unique case, “skey->isUnique()” returns false, and we look >> > at the other conditions in the ‘if’. If the key is partially covered >> > with equality predicates, “skey->areAllChosenPredsEqualPreds()” returns >> > true. >> > The other conditions also return true, and the ‘if’ is taken. So the >> > “trafodion_delete” only plan IS NOT considered. But if there are no >> > key predicates at all (the “delete from t;” example), >> > “skey->areAllChosenPredsEqualPreds()” returns false. >> > >> > >> > >> > I think this is a bug. I think “skey->areAllChosenPredsEqualPreds()” >> > should return true in this case also. It’s an example of the old >> > not-handling-the-empty-set-case-properly bug. >> > >> > >> > >> > I investigated how this particular flag is set. At the end of >> > SearchKeyWorkSpace::computeBeginKeyAndEndKeyValues (in file >> > SearchKey.cpp) is the following code: >> > >> > >> > >> > if ((allChosenPredsAreEqualPreds) && >> > >> > (allBeginKeysMissing) && >> > >> > (allEndKeysMissing)) >> > >> > allChosenPredsAreEqualPreds = FALSE; >> > >> > >> > >> > So, there is explicit code there to treat the empty set case >> differently. >> > So, someone, somewhere, thought this was the right thing to do. >> > >> > >> > >> > But, I’m guessing the right thing to do is to simply delete this code. >> > >> > >> > >> > Does anyone have an opinion? >> > >> > >> > >> > Thanks, >> > >> > >> > >> > Dave >> > >> >> >> >> -- >> Regards, --Qifan >> > > > > -- > Regards, --Qifan > > -- Regards, --Qifan
