Hello, This is a review for MDEV-4309, second patch: https://mariadb.atlassian.net/secure/attachment/21510/201303271828_sanja_cop... === 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