Details
-
Task
-
Status: Stalled (View Workflow)
-
Major
-
Resolution: Unresolved
-
None
Description
Make Value independent from Item, so Item can pass it and change (may be store also).
Why value
We're trying to solve the following goals by adding Value:
- A step to make Items reentrant
- Improve performance
- Reduce the code size by removing some data type specific functions/methods
- Pass data to plugins
The current value implementation
There is already struct st_value and a method in Item:
bool save_in_value(THD *thd, struct st_value *value)
|
{
|
return type_handler()->Item_save_in_value(thd, this, value); |
}
|
Monty will write why the current implementation is bad for the intended goals.
For now it should be:
- Null flag
- reference on Type
- DATA
Attachments
Issue Links
- is part of
-
MDEV-6897 Making Items reentrant
-
- Open
-
Activity
I don't think "reference on Type" should be part of the Value. We already have problems with String and my_decimal that duplicate some of the Item's metadata.
Perhaps the Value object should have only the data and metadata should be in the Item?
serg, this is a good idea. I have only one concern: how getting, say, a double value will look like.
{
|
Value tmp;
|
if (item->val(thd, &val)) |
{
|
handle_null();
|
}
|
else |
{
|
double nr= item->type_handler()->value_to_double(tmp); |
handle_double(nr);
|
}
|
}
|
Something like this?
It looks too cumbersome.
I suggest this API that I proposed by email:
{
|
Double_null nr= item->to_double_null(thd);
|
if (nr.is_null()) |
handle_null();
|
else |
handle_double(nr.val());
|
}
|
Note, Item::to_double_null() can use Value internally in the end:
Double_null Item::to_double_null()
|
{
|
Value tmp;
|
if (val(thd, &tmp)) |
return Double_null(); // This is NULL |
return Double_null(type_handler()->value_to_double(val)); // A non-NULL value |
}
|
The key point is that we can add methods like to_double_null() BEFORE adding Item::val(), thus split a huge change into smaller pieces:
1. Change val_real() to to_double_null()
- a. Add to_double_null(), using the current API: val_real() and null_value,
fix all callers to use to_double_null() instead of val_real() - b. Replace virtual val_real() to virtual to_double_null().
Note, 1a and 1b will be separate small patches.
Iterate these two steps for all non-reentrant methods (again, two patches for each method):
2. Change val_int() to to_longlong_null()
3. Change val_bool() to to_bool_null()
4. Change get_date() and get_time() to:
- Date to_date()
- Time to_time()
- Datetime to_datetime()
5. Remove Item::null_value
After that, Item::val() can be added, if really needed, and all methods to_xxx_null(), to_date(), to_time(), to_datetime() can be modified to use val() internally.
Don't mix too many changes! One change at a time!
How will addition look like with Item::val()?
bool real_op(Value *to) |
{
|
Value a, b;
|
if (args[0]->val(&a) || args[1]->val(&b)) |
return true; |
to->set_double(args[0]->type_handler()->value_to_double(a) +
|
args[1]->type_handler()->value_to_double(b));
|
}
|
The above is cumbersome again.
A much better version:
Double_null double_op()
|
{
|
Double_null a= args[0]->to_double_null();
|
Double_null b= args[1]->to_double_null();
|
return Double_null(a.val() + b.val(), a.is_null() || b.is_null()); |
}
|
Even better version: why not add operator+() to Double_null and hide NULL processing inside, so the caller looks as simple as:
Double_null double_op()
|
{
|
return a->to_double_null() + b->to_double_null(); |
}
|
Just to get the discussion going:
Here is an example of how to separate the null value by adding a pointer to null object to all val functions, except val_str() and val_decimal() who don't need it (a null pointer is a good way to detect the null value for these)
diff --git a/sql/item_func.cc b/sql/item_func.cc
|
index ead72007518..2e10b211c53 100644
|
--- a/sql/item_func.cc
|
+++ b/sql/item_func.cc
|
@@ -1097,12 +1097,15 @@ void Item_real_typecast::print(String *str, enum_query_type query_type)
|
str->append(')');
|
}
|
|
-double Item_func_plus::real_op()
|
+double Item_func_plus::real_op(bool *null_value)
|
{
|
- double value= args[0]->val_real() + args[1]->val_real();
|
- if ((null_value=args[0]->null_value || args[1]->null_value))
|
+ double value1= args[0]->val_real(null_value);
|
+ if (*null_value)
|
return 0.0;
|
- return check_float_overflow(value);
|
+ double value2= args[0]->val_real(null_value);
|
+ if (*null_value)
|
+ return 0.0;
|
+ return check_float_overflow(value+value2);
|
}
|
|
|
diff --git a/sql/item_func.h b/sql/item_func.h
|
index 3771992d617..dd1a9bfe8cd 100644
|
--- a/sql/item_func.h
|
+++ b/sql/item_func.h
|
@@ -739,13 +739,13 @@ class Item_func_hybrid_field_type: public Item_hybrid_func
|
{
|
return str_op_with_null_check(&str_value);
|
}
|
- longlong val_int_from_int_op()
|
+ longlong val_int_from_int_op(bool *null_value)
|
{
|
- return int_op();
|
+ return int_op(null_value);
|
}
|
- double val_real_from_real_op()
|
+ double val_real_from_real_op(bool *null_value)
|
{
|
- return real_op();
|
+ return real_op(null_value);
|
}
|
|
// Value methods that involve conversion
|
@@ -787,17 +787,17 @@ class Item_func_hybrid_field_type: public Item_hybrid_func
|
Item_hybrid_func(thd, list)
|
{ collation= DTCollation_numeric(); }
|
|
- double val_real()
|
+ double val_real(bool *null_value)
|
{
|
DBUG_ASSERT(fixed);
|
return Item_func_hybrid_field_type::type_handler()->
|
- Item_func_hybrid_field_type_val_real(this);
|
+ Item_func_hybrid_field_type_val_real(this, null_value);
|
}
|
- longlong val_int()
|
+ longlong val_int(bool *null_value)
|
{
|
DBUG_ASSERT(fixed);
|
return Item_func_hybrid_field_type::type_handler()->
|
- Item_func_hybrid_field_type_val_int(this);
|
+ Item_func_hybrid_field_type_val_int(this, null_value);
|
}
|
my_decimal *val_decimal(my_decimal *dec)
|
{
|
@@ -1384,8 +1384,8 @@ class Item_func_plus :public Item_func_additive_op
|
const char *func_name() const { return "+"; }
|
enum precedence precedence() const { return ADD_PRECEDENCE; }
|
bool fix_length_and_dec();
|
- longlong int_op();
|
- double real_op();
|
+ longlong int_op(bool *null_value);
|
+ double real_op(bool *null_value);
|
my_decimal *decimal_op(my_decimal *);
|
Item *get_copy(THD *thd)
|
{ return get_item_copy<Item_func_plus>(thd, this); }
|
diff --git a/sql/sql_type.cc b/sql/sql_type.cc
|
index cbf620e85d1..09fc75a99db 100644
|
--- a/sql/sql_type.cc
|
+++ b/sql/sql_type.cc
|
@@ -5219,7 +5219,8 @@ Type_handler_int_result::Item_func_hybrid_field_type_val_str(
|
|
double
|
Type_handler_int_result::Item_func_hybrid_field_type_val_real(
|
- Item_func_hybrid_field_type *item)
|
+ Item_func_hybrid_field_type *item,
|
+ bool *null_value)
|
const
|
{
|
return item->val_real_from_int_op();
|
@@ -5227,11 +5228,11 @@ Type_handler_int_result::Item_func_hybrid_field_type_val_real(
|
|
|
longlong
|
-Type_handler_int_result::Item_func_hybrid_field_type_val_int(
|
- Item_func_hybrid_field_type *item)
|
+Type_handler_int_result::Item_func_hybrid_field_type_val_int( Item_func_hybrid_field_type *item,
|
+ bool *null_value)
|
const
|
{
|
- return item->val_int_from_int_op();
|
+ return item->val_int_from_int_op(null_value);
|
}
|
|
To usage of this would be:
bool null_value=0; /* Only initialization needed in most cases */ |
longlong int= item->val_int(&null_value); |
Note that the plus function is coded slightly different from before to allow shortcutting the + operation if we get a null early
first test is to make Item::val() method which return the value object and see how it will work in points where val_* used not in Item class.