Hello Alexey, Thanks for review! I addressed your suggestions. Also, Vicentiu expressed a concern (when reviewing another patch with the same approach) that this kind of cast that I used in sql_type.cc might be unsafe, because it's non-standard. We're discussion other possible solutions now. So I thought that in the meanwhile for the built-in data types we can create methods: String *val_str_ascii_from_val_int(String *str); String *val_str_ascii_from_val_real(String *str); String *val_str_ascii_from_val_str(String *str); and use them from Type_handler_xxx. A new patch is attached. Please see comments below. On 11/30/2016 05:46 PM, Alexey Botchkov wrote:

Hi, Alexander!

Basically ok with the patch. Few comments though:

...

+String *Item_func_hex::val_str_ascii_from_int(String *str, ulonglong num) { ... + char ans[65], *ptr; + if (!(ptr= longlong2str(num, ans, 16)) || + str->copy(ans, (uint32) (ptr - ans), &my_charset_numeric)) + return make_empty_result(); // End of memory + return str; }

We can avoid extra copying here like this:

{ char *n_end; str->set_charset(&my_charset_numeric); if (str->alloc(65) || !(n_end= longlong2str(num, (char *) str->ptr(), 16))) return make_empty_result(); str->length((uint32) (n_end - str->ptr()); return str; }

Thanks for the idea. It's nice to fix it at once. I moved this to String rather than Item_func_hex though: - this helps to avoid (char*) str->ptr(). - it can be useful for other purposes I tried to move octex2hex() from passport.c to int2str.c and reuse it in sql_string.cc, but failed because of strange compilation failures in mysqlbinlog.cc which includes both password.c and sql_string.cc using #include. From my understanding there is a bug in password.c: instead of #include it should be: #include "mysql.h" I would like to avoid making changes in here for now, so I gave up trying to reuse octet2str(). Instead of that I added APPEND_HEX() and reused it in 3 places in sql_string.cc.

...

class Item_func_hex :public Item_str_ascii_func { ....

Shouldn't we implement this too: ? const Type_handler *Item_func_hex::type_handler() { return m_handler; }

Not really. Type handler for Item_func_hex is type_handler_varchar, and it does not depend on the data type of the argument. To avoid confusion, I renamed m_handler to m_arg0_type_handler.

Personally i wouldn't add the m_handler member at all and just do const Type_handler *Item_func_hex::type_handler() { return args[0]->m_handler; } Are you sure the m_handler member works faster? Well you can just forget that comment by now, but i'd like to test this someday.

I think we should cache it, because it can be expensive to get the handler of args[0] on every row, as it involves virtual methods, which can call more virtual methods recursively. I added a comment about this.

...

+class Item_func_hex_str: public Item_func_hex +{ +public: + String *val_str_ascii(String *str) + { + /* Convert given string to a hex string, character by character */ + String *res= args[0]->val_str(str); + if ((null_value= (!res || tmp_value.alloc(res->length() * 2 + 1)))) + return 0; + tmp_value.length(res->length() * 2); + tmp_value.set_charset(&my_charset_latin1); + octet2hex((char*) tmp_value.ptr(), res->ptr(), res->length()); + return &tmp_value; + } +};

I think it makes more sence to switch the *str and tmp_value usage here. So the caller gets the 'str' value he sends to the function as a result. Not that it changes a lot, still i think it's nicer.

Good idea! Done.

Best regards. HF