List:Commits« Previous MessageNext Message »
From:Øystein Grøvlen Date:October 9 2009 7:17am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3147) Bug#34384
View as plain text  
Evgeny,

Thanks for adressing my comments.  I approve the patch.
Below are some minor comments that you may consider.

Evgeny Potemkin wrote:
 > #At file:///work/bzr_trees/34384-bug-5.1-bugteam/ based on 
revid:kristofer.pettersson@stripped
 >
 >  3147 Evgeny Potemkin	2009-10-08
 >       Bug#34384: Slow down on constant conversion.

...

 > === modified file 'sql/item.h'
 > --- a/sql/item.h	2009-08-28 10:55:59 +0000
 > +++ b/sql/item.h	2009-10-08 11:19:15 +0000
 > @@ -523,6 +523,9 @@ public:
 >                                             of its arguments is or 
contains a
 >                                             subselect */
 >    Item_result cmp_context;              /* Comparison context */
 > +  bool uncacheable;               /* Whether item can be cached by
 > +                                     Arg_comparator. See description of
 > + 
Arg_comparator::cache_converted_constant.*/

I would generalize this comment a bit.  Even if it is currently only
used by Arg_comparator, is there any reason it could not be used in
other scenarios?

...

 >    // alloc & destruct is done as start of select using sql_alloc
 >    Item();
 >    /*
 > @@ -2890,15 +2893,26 @@ protected:
 >    */
 >    Field *cached_field;
 >    enum enum_field_types cached_field_type;
 > -public:
 > -  Item_cache():
 > -    example(0), used_table_map(0), cached_field(0), 
cached_field_type(MYSQL_TYPE_STRING)
 > +  /*
 > +    TRUE <=> cache value on the first val_xxx call.
 > +    In some cases it's not possible to get value of the item to be 
cached at
 > +    the moment of cached being created & initialized. This flag 
tells Item_cache

Typo: "cached" => "the cache"

 > +    only to save the item to be cached without actually caching it.
 > +    On the first call of val_xxx function the value of the item will be
 > +    retrieved, cached and returned to the caller.
 > +  */
 > +  bool late_caching;

...

 > === modified file 'sql/item_cmpfunc.h'
 > --- a/sql/item_cmpfunc.h	2008-12-12 11:13:11 +0000
 > +++ b/sql/item_cmpfunc.h	2009-10-08 11:19:15 +0000
 > @@ -94,6 +94,8 @@ public:
 >                                                        ulonglong 
*const_val_arg);
 >
 >    void set_datetime_cmp_func(Item **a1, Item **b1);
 > +  Item** cache_converted_constant(Item **value, Item **cache,
 > +                                  Item_result type);

I am a bit uncertain about where this function belongs.  AFAICT, it
does not access any of the internals of ArgComparator so it need not
really be part of that class. At least, I do not think it should be
part of the public interface.

-- 
Øystein
Thread
bzr commit into mysql-5.1-bugteam branch (epotemkin:3147) Bug#34384Evgeny Potemkin8 Oct
  • Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3147)Bug#34384Sergei Golubchik8 Oct
    • Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3147) Bug#34384Evgeny Potemkin9 Oct
      • Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3147) Bug#34384Roy Lyseng9 Oct
        • Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3147) Bug#34384Evgeny Potemkin13 Oct
  • Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3147) Bug#34384Øystein Grøvlen9 Oct
  • Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3147)Bug#34384Sergei Golubchik14 Oct