maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hello Sergei,

Please have a look into a modified patch:

https://github.com/MariaDB/server/commit/a507f126b03bfa18fdc7a1d87f368231bb7e58aa


Changes comparing to the previous version:
- Review suggestions addressed
- Added thorough tests for sql_mode='' and for strict+IGNORE.
- Changed the error message, so the user can see the column
  (or the SP variable) name which causes the problem.



On 8/4/22 1:00 AM, Sergei Golubchik wrote:
> Hi, Alexander,
>
> A couple of comments:
<cut>
>
> > -bool Field::check_assignability_from(const Type_handler *from) const
> > +bool Field::check_assignability_from(const Type_handler *from, bool error) const
>
> "error" is very confusing here, I had to read the whole patch to
> understand what it means. Better names:
> * only_warning
> * ignore
> * strict
> * issue_error


Thanks. I chose "ignore". It not only makes the purpose of the parameter
clearer, but also helps to avoid code duplication:

the condition mixing IGNORE and sql_mode flags is now tested
only in one place: when raising an error or a warning.


> >   {
> >     /*
> >       Using type_handler_for_item_field() here to get the data type handler
> > @@ -982,6 +982,15 @@ bool Field::check_assignability_from(const Type_handler *from) const
> >                                         type_handler_for_item_field());
> >     if (th.aggregate_for_result(from->type_handler_for_item_field()))
> >     {
> > +    if (!error)
> > +    {
> > +      THD *thd= get_thd();
> > +      push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN,
> > +                   ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION,
> > +                   ER_THD(thd, ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION),
> > +                   type_handler()->name().ptr(), from->name().ptr(), "SET");
> > +      return false;
> > +    }
> >       my_error(ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION, MYF(0),
> >                type_handler()->name().ptr(), from->name().ptr(), "SET");
> >       return true;
>
> if you wouldn't want to change the error message in a followup commit,
> I'd suggested something like
>
>    my_error(ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION,
>             MYF(error ? 0 : ME_WARNING),
>             type_handler()->name().ptr(), from->name().ptr(), "SET");
>    return error;

Thanks for the idea. I did not know this trick.
And it also works with my_printf_error:


+    bool error= !ignore && get_thd()->is_strict_mode();
+    my_printf_error(ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION,
+                    "Unknown CAST(%s AS %s) in assignment of '%s'",
+                    MYF(error ? 0 : ME_WARNING),
+                    from->name().ptr(), type_handler()->name().ptr(),
+                    field_name.str);


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