maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #10143
Re: Please review MDEV-11298 Split Item_func_hex::val_str_ascii() into virtual methods in Type_handler
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 <mysql.h>
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
diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc
index acd3d74..35bf65c 100644
--- a/sql/item_strfunc.cc
+++ b/sql/item_strfunc.cc
@@ -3621,53 +3621,41 @@ String *Item_func_weight_string::val_str(String *str)
}
-String *Item_func_hex::val_str_ascii(String *str)
+String *Item_func_hex::val_str_ascii_from_val_real(String *str)
{
- String *res;
- DBUG_ASSERT(fixed == 1);
- if (args[0]->result_type() != STRING_RESULT)
- {
- ulonglong dec;
- char ans[65],*ptr;
- /* Return hex of unsigned longlong value */
- if (args[0]->result_type() == REAL_RESULT ||
- args[0]->result_type() == DECIMAL_RESULT)
- {
- double val= args[0]->val_real();
- if ((val <= (double) LONGLONG_MIN) ||
- (val >= (double) (ulonglong) ULONGLONG_MAX))
- dec= ~(longlong) 0;
- else
- dec= (ulonglong) (val + (val > 0 ? 0.5 : -0.5));
- }
- else
- dec= (ulonglong) args[0]->val_int();
+ ulonglong dec;
+ double val= args[0]->val_real();
+ if ((null_value= args[0]->null_value))
+ return 0;
+ if ((val <= (double) LONGLONG_MIN) ||
+ (val >= (double) (ulonglong) ULONGLONG_MAX))
+ dec= ~(longlong) 0;
+ else
+ dec= (ulonglong) (val + (val > 0 ? 0.5 : -0.5));
+ return str->set_hex(dec) ? make_empty_result() : str;
+}
- if ((null_value= args[0]->null_value))
- return 0;
-
- if (!(ptr= longlong2str(dec, ans, 16)) ||
- str->copy(ans,(uint32) (ptr - ans),
- &my_charset_numeric))
- return make_empty_result(); // End of memory
- return str;
- }
- /* Convert given string to a hex string, character by character */
- res= args[0]->val_str(str);
- if (!res || tmp_value.alloc(res->length()*2+1))
- {
- null_value=1;
- return 0;
- }
- null_value=0;
- tmp_value.length(res->length()*2);
- tmp_value.set_charset(&my_charset_latin1);
+String *Item_func_hex::val_str_ascii_from_val_str(String *str)
+{
+ DBUG_ASSERT(&tmp_value != str);
+ String *res= args[0]->val_str(&tmp_value);
+ DBUG_ASSERT(res != str);
+ if ((null_value= (res == NULL)))
+ return NULL;
+ return str->set_hex(res->ptr(), res->length()) ? make_empty_result() : str;
+}
- octet2hex((char*) tmp_value.ptr(), res->ptr(), res->length());
- return &tmp_value;
+
+String *Item_func_hex::val_str_ascii_from_val_int(String *str)
+{
+ ulonglong dec= (ulonglong) args[0]->val_int();
+ if ((null_value= args[0]->null_value))
+ return 0;
+ return str->set_hex(dec) ? make_empty_result() : str;
}
+
/** Convert given hex string to a binary string. */
String *Item_func_unhex::val_str(String *str)
diff --git a/sql/item_strfunc.h b/sql/item_strfunc.h
index 25b63eb..abe0872 100644
--- a/sql/item_strfunc.h
+++ b/sql/item_strfunc.h
@@ -913,17 +913,33 @@ class Item_func_conv :public Item_str_func
class Item_func_hex :public Item_str_ascii_func
{
+protected:
String tmp_value;
+ /*
+ Calling arg[0]->type_handler() can be expensive on every row.
+ It's a virtual method, and in case if args[0] is a complex Item,
+ its type_handler() can call more virtual methods.
+ So let's cache it during fix_length_and_dec().
+ */
+ const Type_handler *m_arg0_type_handler;
public:
Item_func_hex(THD *thd, Item *a):
- Item_str_ascii_func(thd, a) {}
+ Item_str_ascii_func(thd, a), m_arg0_type_handler(NULL) {}
const char *func_name() const { return "hex"; }
- String *val_str_ascii(String *);
+ 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);
+ String *val_str_ascii(String *str)
+ {
+ DBUG_ASSERT(fixed);
+ return m_arg0_type_handler->Item_func_hex_val_str_ascii(this, str);
+ }
void fix_length_and_dec()
{
collation.set(default_charset());
decimals=0;
fix_char_length(args[0]->max_length * 2);
+ m_arg0_type_handler= args[0]->type_handler();
}
Item *get_copy(THD *thd, MEM_ROOT *mem_root)
{ return get_item_copy<Item_func_hex>(thd, mem_root, this); }
diff --git a/sql/sql_string.cc b/sql/sql_string.cc
index 4944226..fd0baae 100644
--- a/sql/sql_string.cc
+++ b/sql/sql_string.cc
@@ -130,6 +130,61 @@ bool String::set_int(longlong num, bool unsigned_flag, CHARSET_INFO *cs)
return FALSE;
}
+
+// Convert a number into its HEX representation
+bool String::set_hex(ulonglong num)
+{
+ char *n_end;
+ if (alloc(65) || !(n_end= longlong2str(num, Ptr, 16)))
+ return true;
+ length((uint32) (n_end - Ptr));
+ set_charset(&my_charset_latin1);
+ return false;
+}
+
+
+/**
+ Append a hex representation of the byte "value" into "to".
+ Note:
+ "to" is incremented for the caller by two bytes. It's passed by reference!
+ So it resembles a macros, hence capital letters in the name.
+*/
+static inline void APPEND_HEX(char *&to, uchar value)
+{
+ *to++= _dig_vec_upper[((uchar) value) >> 4];
+ *to++= _dig_vec_upper[((uchar) value) & 0x0F];
+}
+
+
+void String::qs_append_hex(const char *str, uint32 len)
+{
+ const char *str_end= str + len;
+ for (char *to= Ptr + str_length ; str < str_end; str++)
+ APPEND_HEX(to, (uchar) *str);
+ str_length+= len * 2;
+}
+
+
+// Convert a string to its HEX representation
+bool String::set_hex(const char *str, uint32 len)
+{
+ /*
+ Safety: cut the source string if "len" is too large.
+ Note, alloc() can allocate some more space than requested, due to:
+ - ALIGN_SIZE
+ - one extra byte for a null terminator
+ So cut the source string to 0x7FFFFFF0 rather than 0x7FFFFFFE.
+ */
+ set_if_smaller(len, 0x7FFFFFF0);
+ if (alloc(len * 2))
+ return true;
+ length(0);
+ qs_append_hex(str, len);
+ set_charset(&my_charset_latin1);
+ return false;
+}
+
+
bool String::set_real(double num,uint decimals, CHARSET_INFO *cs)
{
char buff[FLOATING_POINT_BUFFER];
@@ -960,8 +1015,7 @@ my_copy_with_hex_escaping(CHARSET_INFO *cs,
break; /* purecov: inspected */
*dst++= '\\';
*dst++= 'x';
- *dst++= _dig_vec_upper[((unsigned char) *src) >> 4];
- *dst++= _dig_vec_upper[((unsigned char) *src) & 15];
+ APPEND_HEX(dst, (uchar) *src);
src++;
dstlen-= 4;
}
@@ -1146,8 +1200,7 @@ uint convert_to_printable(char *to, size_t to_len,
break;
*t++= '\\';
*t++= 'x';
- *t++= _dig_vec_upper[((unsigned char) *f) >> 4];
- *t++= _dig_vec_upper[((unsigned char) *f) & 0x0F];
+ APPEND_HEX(t, *f);
}
if (t_end - t >= 3) // '...'
dots= t;
diff --git a/sql/sql_string.h b/sql/sql_string.h
index e51d089..bbb384a 100644
--- a/sql/sql_string.h
+++ b/sql/sql_string.h
@@ -293,6 +293,9 @@ class String
bool set(ulonglong num, CHARSET_INFO *cs) { return set_int((longlong)num, true, cs); }
bool set_real(double num,uint decimals, CHARSET_INFO *cs);
+ bool set_hex(ulonglong num);
+ bool set_hex(const char *str, uint32 len);
+
/* Take over handling of buffer from some other object */
void reset(char *ptr_arg, uint32 length_arg, uint32 alloced_length_arg,
CHARSET_INFO *cs)
@@ -569,6 +572,7 @@ class String
qs_append(str, (uint32)strlen(str));
}
void qs_append(const char *str, uint32 len);
+ void qs_append_hex(const char *str, uint32 len);
void qs_append(double d);
void qs_append(double *d);
inline void qs_append(const char c)
diff --git a/sql/sql_type.cc b/sql/sql_type.cc
index 12fd73d..8746595 100644
--- a/sql/sql_type.cc
+++ b/sql/sql_type.cc
@@ -859,3 +859,44 @@ bool Type_handler_temporal_result::
}
/*************************************************************************/
+
+String *
+Type_handler_real_result::Item_func_hex_val_str_ascii(Item_func_hex *item,
+ String *str) const
+{
+ return item->val_str_ascii_from_val_real(str);
+}
+
+
+String *
+Type_handler_decimal_result::Item_func_hex_val_str_ascii(Item_func_hex *item,
+ String *str) const
+{
+ return item->val_str_ascii_from_val_real(str);
+}
+
+
+String *
+Type_handler_int_result::Item_func_hex_val_str_ascii(Item_func_hex *item,
+ String *str) const
+{
+ return item->val_str_ascii_from_val_int(str);
+}
+
+
+String *
+Type_handler_temporal_result::Item_func_hex_val_str_ascii(Item_func_hex *item,
+ String *str) const
+{
+ return item->val_str_ascii_from_val_str(str);
+}
+
+
+String *
+Type_handler_string_result::Item_func_hex_val_str_ascii(Item_func_hex *item,
+ String *str) const
+{
+ return item->val_str_ascii_from_val_str(str);
+}
+
+/*************************************************************************/
diff --git a/sql/sql_type.h b/sql/sql_type.h
index ef85f3d..92dee61 100644
--- a/sql/sql_type.h
+++ b/sql/sql_type.h
@@ -27,6 +27,7 @@ class Field;
class Item;
class Item_cache;
class Item_sum_hybrid;
+class Item_func_hex;
class Type_std_attributes;
class Sort_param;
class Arg_comparator;
@@ -291,6 +292,8 @@ class Type_handler
virtual Item_cache *Item_get_cache(THD *thd, const Item *item) const= 0;
virtual bool set_comparator_func(Arg_comparator *cmp) const= 0;
virtual bool Item_sum_hybrid_fix_length_and_dec(Item_sum_hybrid *) const= 0;
+ virtual String *Item_func_hex_val_str_ascii(Item_func_hex *item,
+ String *str) const= 0;
};
@@ -349,6 +352,11 @@ class Type_handler_row: public Type_handler
DBUG_ASSERT(0);
return true;
}
+ String *Item_func_hex_val_str_ascii(Item_func_hex *item, String *str) const
+ {
+ DBUG_ASSERT(0);
+ return NULL;
+ }
};
@@ -383,6 +391,7 @@ class Type_handler_real_result: public Type_handler_numeric
Item_cache *Item_get_cache(THD *thd, const Item *item) const;
bool set_comparator_func(Arg_comparator *cmp) const;
bool Item_sum_hybrid_fix_length_and_dec(Item_sum_hybrid *func) const;
+ String *Item_func_hex_val_str_ascii(Item_func_hex *item, String *str) const;
};
@@ -402,6 +411,7 @@ class Type_handler_decimal_result: public Type_handler_numeric
Item_cache *Item_get_cache(THD *thd, const Item *item) const;
bool set_comparator_func(Arg_comparator *cmp) const;
bool Item_sum_hybrid_fix_length_and_dec(Item_sum_hybrid *func) const;
+ String *Item_func_hex_val_str_ascii(Item_func_hex *item, String *str) const;
};
@@ -421,6 +431,7 @@ class Type_handler_int_result: public Type_handler_numeric
Item_cache *Item_get_cache(THD *thd, const Item *item) const;
bool set_comparator_func(Arg_comparator *cmp) const;
bool Item_sum_hybrid_fix_length_and_dec(Item_sum_hybrid *func) const;
+ String *Item_func_hex_val_str_ascii(Item_func_hex *item, String *str) const;
};
@@ -438,6 +449,7 @@ class Type_handler_temporal_result: public Type_handler
Item_cache *Item_get_cache(THD *thd, const Item *item) const;
bool set_comparator_func(Arg_comparator *cmp) const;
bool Item_sum_hybrid_fix_length_and_dec(Item_sum_hybrid *func) const;
+ String *Item_func_hex_val_str_ascii(Item_func_hex *item, String *str) const;
};
@@ -459,6 +471,7 @@ class Type_handler_string_result: public Type_handler
Item_cache *Item_get_cache(THD *thd, const Item *item) const;
bool set_comparator_func(Arg_comparator *cmp) const;
bool Item_sum_hybrid_fix_length_and_dec(Item_sum_hybrid *func) const;
+ String *Item_func_hex_val_str_ascii(Item_func_hex *item, String *str) const;
};
References
