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: igor@askmonty.org-20110326053117-x50cah9krd4f1345 parent: wlad@montyprogram.com-20110212174322-f7ptnc0u32vasdwy committer: Igor Babaev <igor@askmonty.org> 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 @@ };

-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;

+ curr_item= (*list_it)++; + } + } + Item* operator++(int) + { + List_iterator_fast<Item> *list_it= (List_iterator_fast<Item> *) this;

+ 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: Please add a one-line comment describing what this class does. psergey: Why do redundant explicit typecasting? psergey: Why do redundant explicit typecasting? 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