maria-developers team mailing list archive

[Sergey Petrunya <psergey@xxxxxxxxxxxx>]

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@xxxxxxxxxxxx-20110326053117-x50cah9krd4f1345
> parent: wlad@xxxxxxxxxxxxxxxx-20110212174322-f7ptnc0u32vasdwy
> committer: Igor Babaev <igor@xxxxxxxxxxxx>
> 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