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
