maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

On Jan 17, Alexander Barkov wrote:
> >> diff --git a/sql/item_jsonfunc.cc b/sql/item_jsonfunc.cc
> >> index ddf5fc32ea4..81003be4656 100644
> >> --- a/sql/item_jsonfunc.cc
> >> +++ b/sql/item_jsonfunc.cc
> >> @@ -1441,6 +1441,32 @@ longlong Item_func_json_contains_path::val_int()
...
> >> +static bool is_json_type(const Item *item)
> >> +{
> >> +  for ( ; ; )
> >> +  {
> >> +    if (Type_handler_json_common::is_json_type_handler(item->type_handler()))
> >> +      return true;
> >> +    const Item_func_conv_charset *func;
> >> +    if (!(func= dynamic_cast<const Item_func_conv_charset*>(item)))
> >> +      return false;
> >> +    item= func->arguments()[0];
> > 
> > can you have nested CONVERT()'s ?
> 
> We cannot have auto-generated nested CONVERT().
> 
> The new code just reproduces the old behavior:
> Item_func_conv_charset::is_json_type() traversed
> through all Item_func_conv_charset's recursively.
> 
> As the comment says, this is probably not correct.
> Let's fix it under terms of a separate MDEV.
> I can file an MDEV about this.

please, do

> >> 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;
> > 
> > that's an unusual semantics, not what I would expect from an iterator.
> > It'd expect it to iterare with it++; or it.next(); but not it = it.base();
> 
> I agree it did not look nice.
> 
> I replaced Recursive_type_pair_iterator to a more
> generic new class:
> 
> class Type_handler_pair
> {
>    const Type_handler *m_a;
>    const Type_handler *m_b;
> ...
> };

why did you do it this way? You create an object. Then in the loop you
create a new object, copying the old one. Then you conditionally assign
m_a->type_handler_base(). Then you diff then to see if any condition
from the previous step has succeeded.

Why not, like

  class Type_handler_pair {
    bool up() {
      bool r=false;
      n_a=m_a->type_handler_base();
      n_b=m_b->type_handler_base();
      if (n_a) { m_a=n_a; r=true; }
      if (n_b) { m_a=n_b; r=true; }
      return r;
    }

and later

   do {
      ...
   } while (tp.up());

also don't forget to update the commit comment, it still says
"Recursive_type_pair_iterator".

> >> +  }

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