Hello Igor, First, an overall comment: there are lots of typos/coding style violations in the patch. To reduce amount of effort spent on such things, I was just fixing them as I saw them and I'm attaching the patch with all the fixes (i.e. this patch should be applied on top of the patch that I was reviewing).
More important comments are bellow, marked with 'psergey:' On Fri, Mar 25, 2011 at 10:31:19PM -0700, Igor Babaev wrote: > At file:///home/igor/maria/maria-5.3-bug717577/ > > ------------------------------------------------------------ > revno: 2908 > revision-id: [email protected] > parent: [email protected] > committer: Igor Babaev <[email protected]> > branch nick: maria-5.3-bug717577 > timestamp: Fri 2011-03-25 22:31:17 -0700 > message: > Fixed LP bugs #717577, #724942. > > Both these two bugs happened due to the following problem. > When a view column is referenced in the query an Item_direct_view_ref > object is created that is refers to the Item_field for the column. > All references to the same view column refer to the same Item_field. > Different references can belong to different AND/OR levels and, > as a result, can be included in different Item_equal object. > These Item_equal objects may include different constant objects. > If these constant objects are substituted for the Item_field created > for a view column we have a conflict situation when the second > substitution annuls the first substitution. This leads to > wrong result sets returned by the query. Bug #724942 demonstrates > such an erroneous behaviour. > Test case of the bug #717577 produces wrong result sets because best > equal fields of the multiple equalities built for different OR levels > of the WHERE condition differs. The subsitution for the best equal field > in the second OR branch overwrites the the substitution made for the > first branch. > > To avoid such conflicts we have to substitute for the references > to the view columns rather than for the underlying field items. > To make such substitutions possible we have to include into > multiple equalities references to view columns rather than > field items created for such columns. > > This patch modifies the Item_equal class to include references > to view columns into multiple equality objects. It also performs > a clean up of the class methods and adds more comments. The methods > of the Item_direct_view_ref class that assist substitutions for > references to view columns has been also added by this patch. > === modified file 'sql/item.cc' > --- a/sql/item.cc 2011-02-01 03:33:32 +0000 > +++ b/sql/item.cc 2011-03-26 05:31:17 +0000 > @@ -7226,6 +7233,129 @@ > return FALSE; > } > > + > +Item_equal *Item_direct_view_ref::find_item_equal(COND_EQUAL *cond_equal) > +{ > + Item* field_item= real_item(); > + if (field_item->type() != FIELD_ITEM) > + return NULL; > + return ((Item_field *) field_item)->find_item_equal(cond_equal); > +} > + > + > +/** > + Check whether a reference to field item can be substituted b an equal item > + > + @details > + The function checks whether a substitution of a reference to field item for > + an equal item is valid. > + > + @param arg *arg != NULL && **arg <-> the reference is in the context > + where substitution for an equal item is valid > + > + @note > + See also the note for Item_field::subst_argument_checker > + > + @retval > + TRUE substitution is valid > + @retval > + FALSE otherwise > +*/ > +bool Item_direct_view_ref::subst_argument_checker(uchar **arg) > +{ > + bool res= (!(*arg) && (result_type() != STRING_RESULT)) || > + ((*arg) && (**arg)); > + if (*arg) > + **arg= (uchar) 0; psergey: What is the above for? Do I understand it correctly that this is needed so that Item_field that this item is wrapping is not substituted? Please add a comment. > + return res; > +} > + > + > +/** > + Set a pointer to the multiple equality the view field reference belongs to > + (if any). > + > + @details > + The function looks for a multiple equality containing this item of the type > + Item_direct_view_ref among those referenced by arg. > + In the case such equality exists the function does the following. > + If the found multiple equality contains a constant, then the item > + is substituted for this constant, otherwise the function sets a pointer > + to the multiple equality in the item. > + > + @param arg reference to list of multiple equalities where > + the item (this object) is to be looked for > + > + @note > + This function is supposed to be called as a callback parameter in calls > + of the compile method. > + > + @note > + The function calls Item_field::equal_fields_propagator for the field item > + this->real_item() to do the job. Then it takes the pointer to equal_item > + from this field item and assigns it to this->item_equal. > + > + @return > + - pointer to the replacing constant item, if the field item was > substituted > + - pointer to the field item, otherwise. > +*/ > + > +Item *Item_direct_view_ref::equal_fields_propagator(uchar *arg) > +{ > + Item *field_item= real_item(); > + if (field_item->type() != FIELD_ITEM) > + return this; > + Item *item= field_item->equal_fields_propagator(arg); > + set_item_equal(field_item->get_item_equal()); > + field_item->set_item_equal(0); psergey: Please use NULL here, we use that for pointers. > + if (item != field_item) > + return item; > + return this; > +} > + ... > === modified file 'sql/item_cmpfunc.cc' > --- a/sql/item_cmpfunc.cc 2011-02-06 04:57:03 +0000 > +++ b/sql/item_cmpfunc.cc 2011-03-26 05:31:17 +0000 > @@ -5516,43 +5516,93 @@ > return 0; > } > > -Item_equal::Item_equal(Item_field *f1, Item_field *f2) > - : Item_bool_func(), const_item(0), eval_item(0), cond_false(0), > - compare_as_dates(FALSE) > -{ > - const_item_cache= 0; > - fields.push_back(f1); > - fields.push_back(f2); > -} > - > -Item_equal::Item_equal(Item *c, Item_field *f) > + > +/** > + Construct a minimal multiple equality item > + > + @param f1 the first equal item > + @param f2 the second equal item > + @param with_const_item TRUE if the first item is constant > + > + @details > + The constructor builds a new item equal object for the equality f1=f2. > + One if the equal items can be constant. If this is the case it is passed > + always as the first parameter and the parameter with_const_item serves > + as an indicator of this case. > + Currently any non-constant parameter items must refer to an item of the > + of the type FIELD_ITEM. psergey: The above is still true for the case where passed item is an Item_direct_view_ref which refers to an Item_field. The wording may be confusing for the new readers, though. Please change to explicitly indicate that f1 and f2 may point to Item_field or Item_direct_view_ref(Item_field) > +*/ > + > +Item_equal::Item_equal(Item *f1, Item *f2, bool with_const_item) > : Item_bool_func(), eval_item(0), cond_false(0) > { > const_item_cache= 0; > - fields.push_back(f); > - const_item= c; > - compare_as_dates= f->is_datetime(); > + with_const= with_const_item; > + compare_as_dates= with_const_item && f2->is_datetime(); > + equal_items.push_back(f1); > + equal_items.push_back(f2); > } > > > +/** > + Copy constructor for a multiple equality > + > + @param item_equal source item for the constructor > + > + @details > + The function creates a copy of an Item_equal object. > + This constructor is used when an item belongs to a multiple equality > + of an upper level (an upper AND/OR level or an upper level of a nested > + outer join). > +*/ > + > Item_equal::Item_equal(Item_equal *item_equal) > : Item_bool_func(), eval_item(0), cond_false(0) > { > const_item_cache= 0; > - List_iterator_fast<Item_field> li(item_equal->fields); > - Item_field *item; > + List_iterator_fast<Item> li(item_equal->equal_items); > + Item *item; > while ((item= li++)) > { > - fields.push_back(item); > + equal_items.push_back(item); > } > - const_item= item_equal->const_item; > + with_const= item_equal->with_const; > compare_as_dates= item_equal->compare_as_dates; > cond_false= item_equal->cond_false; > } > > > -void Item_equal::compare_const(Item *c) > +/* > + @brief > + Add a constant item to the Item_equal object > + > + @param[in] c the constant to add > + @param[in] f item from the list equal_items the item c is equal to > + (this parameter is optional) > + > + @details > + The method adds the constant item c to the list equal_items. If the list > + hasn't not contained any constant item yet the item c is just added > + to the very beginning of the list. Otherwise the value of c is compared > + with the value of the constant item from equal_items. If they are not > + equal cond_false is set to TRUE. This serves as an indicator that this > + Item_equal is always FALSE. psergey: Does function comment really need to duplicate the function body? In my opinion, the above should be split into multiple comments inside the function body. > + The optional parameter f is used to adjust the flag compare_as_dates. > +*/ > + > +void Item_equal::add_const(Item *c, Item *f) > { > + if (cond_false) > + return; > + if (!with_const) > + { > + with_const= TRUE; > + if (f) > + compare_as_dates= f->is_datetime(); > + equal_items.push_front(c); > + return; > + } > + Item *const_item= get_const(); > if (compare_as_dates) > { > cmp.set_datetime_cmp_func(this, &c, &const_item); ... > @@ -5811,19 +5911,15 @@ > return Item_func::transform(transformer, arg); > } > > + > void Item_equal::print(String *str, enum_query_type query_type) > { > str->append(func_name()); > str->append('('); > - List_iterator_fast<Item_field> it(fields); > + List_iterator_fast<Item> it(equal_items); > Item *item; > - if (const_item) > - const_item->print(str, query_type); > - else > - { > - item= it++; > - item->print(str, query_type); > - } > + item= it++; > + item->print(str, query_type); > while ((item= it++)) > { > str->append(','); > @@ -5834,6 +5930,14 @@ > } > > > +CHARSET_INFO *Item_equal::compare_collation() psergey: One expects compare_xxx() functions to compare something, in particular this function looks like it's going to compare some collation. Could you please rename it to e.g. comparison_collation()? > +{ > + Item_equal_fields_iterator it(*this); > + Item *item= it++; > + return item->collation.collation; > +} > + > + > /* > @brief Get the first equal field of multiple equality. > @param[in] field the field to get equal field to ... > === modified file 'sql/item_cmpfunc.h' > --- a/sql/item_cmpfunc.h 2010-10-10 14:18:11 +0000 > +++ b/sql/item_cmpfunc.h 2011-03-26 05:31:17 +0000 > @@ -1672,23 +1707,47 @@ > }; > > psergey: Please add a one-line comment describing what this class does. > -class Item_equal_iterator : public List_iterator_fast<Item_field> > +class Item_equal_fields_iterator : public List_iterator_fast<Item> > { > + Item_equal *item_equal; > + Item *curr_item; > public: > - inline Item_equal_iterator(Item_equal &item_equal) > - :List_iterator_fast<Item_field> (item_equal.fields) > - {} > - inline Item_field* operator++(int) > - { > - Item_field *item= (*(List_iterator_fast<Item_field> *) this)++; > - return item; > - } > - inline void rewind(void) > - { > - List_iterator_fast<Item_field>::rewind(); > - } > + Item_equal_fields_iterator(Item_equal &item_eq) > + :List_iterator_fast<Item> (item_eq.equal_items) > + { > + curr_item= NULL; > + item_equal= &item_eq; > + if (item_eq.with_const) > + { > + List_iterator_fast<Item> *list_it= (List_iterator_fast<Item> *) this; psergey: Why do redundant explicit typecasting? > + curr_item= (*list_it)++; > + } > + } > + Item* operator++(int) > + { > + List_iterator_fast<Item> *list_it= (List_iterator_fast<Item> *) this; psergey: Why do redundant explicit typecasting? > + curr_item= (*list_it)++; > + return curr_item; > + } > + Item ** ref() > + { > + return List_iterator_fast<Item>::ref(); > + } > + void rewind(void) > + { > + List_iterator_fast<Item> *list_it= (List_iterator_fast<Item> *) this; > + list_it->rewind(); > + if (item_equal->with_const) > + curr_item= (*list_it)++; > + } > + Field *get_curr_field() > + { > + Item_field *item= (Item_field *) (curr_item->real_item()); > + return item->field; > + } > }; > > + > class Item_cond_and :public Item_cond > { > public: > > === modified file 'sql/sql_select.cc' > --- a/sql/sql_select.cc 2011-02-11 10:27:35 +0000 > +++ b/sql/sql_select.cc 2011-03-26 05:31:17 +0000 > @@ -9623,7 +9630,8 @@ > args->concat((List<Item> *)&cond_equal.current_level); > } > } > - else if (cond->type() == Item::FUNC_ITEM) > + else if (cond->type() == Item::FUNC_ITEM || > + cond->real_item()->type() == Item::FIELD_ITEM) psergey: As far as I understand the last part of the condition is there so that direct references like ... AND view.column AND ... are processed with subst_argument_checker/equal_fields_propagator. Is it intentional that direct references like ... AND base_table.column AND ... are now processed with subst_argument_checker/equal_fields_propagator as well? > { > List<Item> eq_list; > /* -- BR Sergey -- Sergey Petrunia, Software Developer Monty Program AB, http://askmonty.org Blog: http://s.petrunia.net/blog _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
