Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-12338

Split Item_type_holder::get_real_type() into virtual Item::real_type_handler()

Details

    Description

      Item_type_holder::get_real_type() has a switch(item->type()).

      This coding style makes it impossible for user defined data type plugins to create their own Item types (e.g. descendants of Item_basic_constant, Item_func, Item_sum for data type specific literals and functions, etc) with a desired UNION aggregation behavior.

      Also, this coding style is error prone, because it's very easy to forget to modify Item_type_holder::get_real_type properly when adding new built-in Items.

      We'll do the following:

      1. Introduce a new virtual method in Item:

      virtual const Type_handler *Item::real_type_handler() const;
      

      2. Move pieces from Item_type_holder::get_real_type() "case"s into virtual implementations of the corresponding Items:

      Item::real_type_handler();
      Item_field::real_type_handler();
      Item_blob::real_type_handler();
      Item_ref::real_type_handler();
      Item_type_holder::real_type_handler();
      Item_sum_hybrid::real_type_handler();
      

      Note, there is no a need for Item_func_get_user_var::real_type_handler(). The "case" corresponding to Item_func_get_user_var is not really needed and is a useless code, because starting from 10.1 Item_func_get_user_var is a descendant of Type_handler_hybrid_field_type, which makes sure that:

      Item_func_get_user_var::result_type()
      Item_func_get_user_var::field_type()
      Item_func_get_user_var::type_handler()
      

      are always in sync to each other. All these methods use Type_handler_hybrid_field_type::m_type_handler to return the result.

      In 10.0 and earlier, cmp_type and field_type were stored in separate members:

      Item_result cached_result_type;
      enum_field_types cached_field_type;
      

      which could get out of sync in some cases.

      3. Remove Item_type_holder::get_real_type()

      4. Remove the convenience class Type_handler_hybrid_real_field_type.

      We'll derive Item_type_holder from Type_handler_hybrid_field_type, because the new method Item::real_type_handler() makes it easy to use Type_handler_hybrid_field_type in Item_type_holder directly.

      5. Remove Item_sum::keep_field_type()

      The idea of keep_field_type() is that the aggregate functions MAX() and MIN() try to preserve the exact data type (e.g. ENUM or SET) from the argument when they appear as a part of a UNION, e.g in combinations with NULL:

      Notice, this script creates a column of the ENUM data type in the table t2:

      CREATE OR REPLACE TABLE t1 (a ENUM('a','b'));
      CREATE OR REPLACE TABLE t2 AS SELECT MAX(a) FROM t1 UNION SELECT NULL;
      SHOW CREATE TABLE t2;
      

      +-------+--------------------------------------------------------------------------------------------------+
      | Table | Create Table                                                                                     |
      +-------+--------------------------------------------------------------------------------------------------+
      | t2    | CREATE TABLE `t2` (
        `MAX(a)` enum('a','b') DEFAULT NULL
      ) ENGINE=InnoDB DEFAULT CHARSET=latin1 |
      +-------+--------------------------------------------------------------------------------------------------+
      

      Unlike MIN and MAX, most functions convert ENUM/SET to VARCHAR.

      keep_field_type() is not needed after adding of real_type_handler(), which replaces this functionality in a more universal way.

      Attachments

        Issue Links

          Activity

            Hi Alexander!

            The patch looks good to me. I would like some test cases, especially for the case FUNC_ITEM that was removed. Ok to push if you believe there are already test cases like that in our suites.

            Now, after reviewing it I found 2 things that should be addressed outside this patch.

            1: Type_handler::aggregate_for_result_traditional is defined in sql/sql_type.h and implemented in sql/field.cc. Can we move it to sql/sql_type.cc? Do you have a strong objection to why it should stay in sql/field.cc?
            2. When I implemented Item_sum_hybrid_simple for window functions I may have made a few mistakes when it comes to type handling. I mostly copied Item_sum_hybrid and deleted parts I did not need. I'll look into cleaning this up, but whenever you're touching that bit of code, feel free to ping me and discuss it.

            cvicentiu Vicențiu Ciorbaru added a comment - Hi Alexander! The patch looks good to me. I would like some test cases, especially for the case FUNC_ITEM that was removed. Ok to push if you believe there are already test cases like that in our suites. Now, after reviewing it I found 2 things that should be addressed outside this patch. 1: Type_handler::aggregate_for_result_traditional is defined in sql/sql_type.h and implemented in sql/field.cc. Can we move it to sql/sql_type.cc? Do you have a strong objection to why it should stay in sql/field.cc? 2. When I implemented Item_sum_hybrid_simple for window functions I may have made a few mistakes when it comes to type handling. I mostly copied Item_sum_hybrid and deleted parts I did not need. I'll look into cleaning this up, but whenever you're touching that bit of code, feel free to ping me and discuss it.

            Hi Vicentiu!

            The UNION behavior of Item_func_get_user_var is covered in tests union.test and metadata.test.
            Btw, I just tried to remove the FUNC_ITEM related code in 10.0, and no mtr tests failed. So it seems it's been a useless code for a long time.

            1. We can move it. I left it in field.cc for easier merge purposes. But now I think it was better to move it.
            2. Sure, I'll discuss if I have to change window function related code next time.

            Thanks for review!

            bar Alexander Barkov added a comment - Hi Vicentiu! The UNION behavior of Item_func_get_user_var is covered in tests union.test and metadata.test. Btw, I just tried to remove the FUNC_ITEM related code in 10.0, and no mtr tests failed. So it seems it's been a useless code for a long time. 1. We can move it. I left it in field.cc for easier merge purposes. But now I think it was better to move it. 2. Sure, I'll discuss if I have to change window function related code next time. Thanks for review!

            People

              bar Alexander Barkov
              bar Alexander Barkov
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

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