← Back to team overview

maria-developers team mailing list archive

Re: f3fc58149dd: MDEV-29159 Patch for MDEV-28918 introduces more inconsistency than it solves, breaks usability


Hello Sergei,

Please have a look into a modified patch:


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:

-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
    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,
+                   type_handler()->name().ptr(), from->name().ptr(), "SET");
+      return false;
+    }
               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

            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();
+                    "Unknown CAST(%s AS %s) in assignment of '%s'",
+                    MYF(error ? 0 : ME_WARNING),
+                    from->name().ptr(), type_handler()->name().ptr(),
+                    field_name.str);

VP of MariaDB Server Engineering
and security@xxxxxxxxxxx