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

Hi, Alexander, A couple of comments: On Aug 03, Alexander Barkov wrote:
"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
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; Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org

Hello Sergei, Please have a look into a modified patch: https://github.com/MariaDB/server/commit/a507f126b03bfa18fdc7a1d87f368231bb7... 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>
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.
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);
participants (2)
-
Alexander Barkov
-
Sergei Golubchik