maria-developers team mailing list archive

[Sergei Petrunia <psergey@xxxxxxxxxxxx>]

Hello,

This is a review for MDEV-4309, second patch:
https://mariadb.atlassian.net/secure/attachment/21510/201303271828_sanja_copy_calls_patch.diff

===
The name "memcmp_func" is confusing, because the function
has nothing to do with memcmp. Did you mean memcpy actually?

The function should be renamed to something like "memcpy_field_value".
memcmp_func() still makes one virtual call.

===
Consider a scenario:
- Item_field::setup_cpy_optimizer() is called. 
- It calls memcmp_possible(), which returns FALSE, which means field_conv()
  will be called for every row.
- field_conv() will still call memcpy_field_value(), even if we know that it
  will return false.

Why not add a field_conv_incompatible() and make field_conv look like this:

  find_conv()
  {
    if (memcmp_possible())
    {
      ...
    }
    return field_conv_incompatible();
  }

===
The name "cpy_optimizer" is very cryptic. Is "Fast_field_copier" better?

===
class Item_field has this member:

  Field *cpy_optimizer_setup;

I don't see how it is useful without its corresponding 'cpy_optimizer' data.  
cpy_optimizer is only present inside ORDER structure which has both:

  cpy_optimizer cpy_optimizer_func;
  Field  *cpy_optimizer_setup;

If ORDER has both members, why have the Item_field::cpy_optimizer_setup ?
===

Let's discuss this on IRC.

BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB | Skype: sergefp | Blog: http://s.petrunia.net/blog