Re: Bug#30946

[Alexander Barkov]

Hi Gleb,

I reviewed the patch at:

http://lists.mysql.com/commits/76888


It looks ok to push.
I have only a few small comments:


1. In file sql/sql_string.cc:

+    else if (to_end - to >= 4) // \xXX
+    {
+      *to++= '\\';
+      *to++= 'x';
+      *to++= _dig_vec_upper[((unsigned char) *from) >> 4];
+      *to++= _dig_vec_upper[((unsigned char) *from) & 0x0F];
+    }


It seems it should be:


+    else
+    {
+      if (to_end - to < 4) // \xXX
+        break;
+      *to++= '\\';
+      *to++= 'x';
+      *to++= _dig_vec_upper[((unsigned char) *from) >> 4];
+      *to++= _dig_vec_upper[((unsigned char) *from) & 0x0F];
+    }

I know you just moved this code from another place, but
could you fix it at once please?



2. In file sql/sql_load.cc:

+  if (escaped->length() > 1 || !my_isascii(escaped->ptr()[0]) ||
+      enclosed->length() > 1 || !my_isascii(enclosed->ptr()[0]) ||
+      !field_term->is_ascii() || !ex->line_term->is_ascii() ||
+      !ex->line_start->is_ascii())
+  {
+    push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
+                 WARN_NON_ASCII_SEPARATOR_NOT_IMPLEMENTED,
+                 ER(WARN_NON_ASCII_SEPARATOR_NOT_IMPLEMENTED));
+  }


Should not the above code be split into two separate warnings,
like you did in sql_class.cc:


  /* First, report problems with multi-byte separators */
  if (exchange->escaped->numchars() > 1 ||
      exchange->enclosed->numchars() > 1)
  {
    push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
                 ER_WRONG_FIELD_TERMINATORS,
                 ER(ER_WRONG_FIELD_TERMINATORS)));
  }

  /* Then report problems with non-ascii separators */
  if (!my_isascii(exchange->escaped->ptr()[0]) ||
      !my_isascii(exchange->enclosed->ptr()[0]) ||
      !exchange->field_term->is_ascii() ||
      !exchange->line_term->is_ascii() ||
      !exchange->line_start->is_ascii())
  {
     push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
                  WARN_NON_ASCII_SEPARATOR_NOT_IMPLEMENTED,
                  ER(WARN_NON_ASCII_SEPARATOR_NOT_IMPLEMENTED));
  }



Otherwise Ok to push for me.