Details

    • Task
    • Status: Stalled (View Workflow)
    • Major
    • Resolution: Unresolved
    • 11.2(EOL)
    • Server
    • 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:

      1. A step to make Items reentrant
      2. Improve performance
      3. Reduce the code size by removing some data type specific functions/methods
      4. 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

          Activity

            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.

            sanja Oleksandr Byelkin added a comment - 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.

            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 Sergei Golubchik added a comment - 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?
            bar Alexander Barkov added a comment - - edited

            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!

            bar Alexander Barkov added a comment - - edited 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!
            bar Alexander Barkov added a comment - - edited

            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();
            }
            

            bar Alexander Barkov added a comment - - edited 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(); }
            monty Michael Widenius added a comment - - edited

            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

            monty Michael Widenius added a comment - - edited 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

            People

              sanja Oleksandr Byelkin
              sanja Oleksandr Byelkin
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

                Created:
                Updated:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.