maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

> - We'll probably have more FORMATs in the future.

Like what? As far as I can see, standard doesn't support FORMAT in the
column definition at all.

>    Adding all format tests inside "normal"
>    Field_string/Field_varchar/Field_blob
>    is not a good idea.
> 
> >> +  */
> >> +  if (Type_handler_json_common::has_json_valid_constraint(this))
> > 
> > could you please use names that don't depend on how exactly we detect
> > json? especially if the goal is to move to FORMAT JSON syntax
> 
> For now this name reflects what it actually does.
> 
> When we split JSON off and fix the code to have JSON put extra
> data type information into FRM, this test will be gone from
> Field_xxx.
> 
> But it will move to the code opening FRM, to be able open
> old tables where JSON is detected only by a CHECK
> constraint. And there it will reflect what it actually
> does again.
> 
> I propose to keep it as is.

ok

> >> diff --git a/sql/sql_type.cc b/sql/sql_type.cc
> >> index c1801c1ae3e..3b2753d80a6 100644
> >> --- a/sql/sql_type.cc
> >> +++ b/sql/sql_type.cc
> >> @@ -1755,19 +1755,110 @@ const Type_handler *Type_handler_typelib::cast_to_int_type_handler() const
> >>   
> >>   /***************************************************************************/
> >>   
> >> +class Recursive_type_pair_iterator
> >> +{
> >> +  const Type_handler *m_a;
> >> +  const Type_handler *m_b;
> >> +  uint m_switched_to_base_count;
> >> +public:
> >> +  Recursive_type_pair_iterator(const Type_handler *a,
> >> +                               const Type_handler *b,
> >> +                               uint switched_to_base_count= 0)
> >> +   :m_a(a), m_b(b), m_switched_to_base_count(switched_to_base_count)
> >> +  { }
> >> +  const Type_handler *a() const { return m_a; }
> >> +  const Type_handler *b() const { return m_b; }
> >> +  Recursive_type_pair_iterator base() const
> >> +  {
> >> +    Recursive_type_pair_iterator res(m_a->type_handler_base(),
> >> +                                     m_b->type_handler_base());
> >> +    res.m_switched_to_base_count= (res.m_a != NULL) + (res.m_b != NULL);
> >> +    if (res.m_a == NULL)
> >> +      res.m_a= m_a;
> >> +    if (res.m_b == NULL)
> >> +      res.m_b= m_b;
> >> +    return res;
> >> +  }
> >> +  bool done() const
> >> +  {
> >> +    switch (m_switched_to_base_count)
> >> +    {
> >> +    case 2:
> >> +      /*
> >> +        Expression:
> >> +          COALESCE(TYPE1, TYPE2)
> >> +
> >> +        where both type handlers derive from some other handlers, e.g.
> >> +          VARCHAR -> TYPE1
> >> +          TEXT    -> TYPE2
> >> +
> >> +        Continue aggregation as:
> >> +          SELECT COALESCE(VARCHAR, TEXT)
> >> +
> >> +        Note, we now have only one level of data type inheritance,
> >> +          e.g. VARCHAR -> VARCHAR/JSON
> >> +
> >> +        This code branch will change when we have longer inheritance chains:
> >> +          A0 -> A1 -> A2 -> A3
> >> +          B0 -> B1 -> B2
> >> +
> >> +        Functions like COALESE(A2, B2) will need to do full overload resolution:
> >> +        - iterate through all pairs from the below matrix
> >> +        - find all existing candidates
> >> +        - resolve ambiguities in case multiple candidates, etc.
> > 
> > and I don't see how you're going to do that. you need to try
> > m_a+m_b.base(), m_a.base()+m_b, and m_a.base()+m_b.base().
> > And that's not what you're doing.
> > 
> > I suggest to add an assert that m_switched_to_base_count < 2
> > and stop trying to solve complex cases until we have at least one.
> 
> Yes, we'll need to do all these combinations eventually.
> But I'm not trying to solve a more complex case
> than it's really necessary now:
> 
> If the SQL query is:
> 
>    SELECT varchar_json_expr + text_json_expr FROM t1;
> 
> it works as follows:
> 
> - It tries VARCHAR/JSON+TEXT/JSON in Type_collection_json,
>    nothing is found
> 
> - Then it tries directly VARCHAR+TEXT,
>    which is m_a.base()+m_b.base(), and this
>    combination is found in Type_collection_std,

I'd expect it to find VARCHAR/JSON+TEXT/JSON right away.
It would've naturally preserved the json property.

Otherwise you'd need to do an extra json propagation.

Here, of course, I mean not Item_func_plus, but, say, COALESCE. Or UNION.

> >> diff --git a/sql/sql_type_json.cc b/sql/sql_type_json.cc
> >> index a804366ec03..a84f720bcc9 100644
> >> --- a/sql/sql_type_json.cc
> >> +++ b/sql/sql_type_json.cc
> >> @@ -20,17 +20,111 @@
> >>   #include "sql_class.h"
> >>   
> >>   
> >> -Type_handler_json_longtext  type_handler_json_longtext;
> >> +Named_type_handler<Type_handler_string_json>
> >> +  type_handler_string_json("char/json");
> > 
> > do you mean these names are not shown anywhere?
> 
> They are shown in error messages, e.g.:
> 
> Illegal parameter data types inet6 and longblob/json for operation '+'

eh, okay. may be we should change it later
to "longblob format json". But not now.

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx