← Back to team overview

maria-developers team mailing list archive

MDEV-4309, second patch: review



This is a review for MDEV-4309, second patch:

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:

    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.

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