i do and also review the orignal branch unique_index_sachin i added more test cases and reinsert also works
On Sun, Aug 14, 2016 at 1:46 PM, Sergei Golubchik <
serg@mariadb.org> wrote:
>
> Hi, Sachin!
>
> I'm reviewing.
> Here I only comment on your reply to the previous review.
>
> > >> +longlong Item_func_hash::val_int()
> > >> +{
> > >> + unsigned_flag= true;
> > >> + ulong nr1= 1,nr2= 4;
> > >> + CHARSET_INFO *cs;
> > >> + for(uint i= 0;i<arg_count;i++)
> > >> + {
> > >> + String * str = args[i]->val_str();
> > >> + if(args[i]->null_value)
> > >> + {
> > >> + null_value= 1;
> > >> + return 0;
> > >> + }
> > >> + cs= str->charset();
> > >> + uchar l[4];
> > >> + int4store(l,str->length());
> > >> + cs->coll->hash_sort(cs,l,sizeof(l), &nr1, &nr2);
> > > looks good, but use my_charset_binary for the length.
> > did not get it :(
>
> You use
>
> cs->coll->hash_sort(cs,l,sizeof(l), &nr1, &nr2);
>
> to sort the length, the byte value of str->length().
> This is binary data, you should have
>
> cs= my_charset_binary;
i do not find any variable name my_charset_binary but i used
my_charset_utf8_bin is it ok ?
>
> but you have
>
> cs= str->charset();
>
> for example, it can be latin1_general_ci, and then 'a' and 'A' will be
> considered equal. But 'a' is the string length 97, while 'A' is string
> length 65. String lengths are binary data, they do not consist of
> "letters" or "characters", so you need to use binary charset for
> lengths, but str->charset() for actual string data.
>
> > >> + cs->coll->hash_sort(cs, (uchar *)str->ptr(), str->length(), &nr1, &nr2);
>
> ^^^ here cs= str->charset() is correct.
>
> > >> + }
> > >> + return (longlong)nr1;
> > >> +}
> > >> +
> > >> +
> > >> diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> > >> index e614692..4872d20 100644
> > >> --- a/sql/sql_yacc.yy
> > >> +++ b/sql/sql_yacc.yy
> > >> | COMMENT_SYM TEXT_STRING_sys { Lex->last_field->comment= $2; }
> > >> + | HIDDEN
> > >> + {
> > >> + LEX *lex =Lex;
> > > no need to do that ^^^ you can simply write Lex->last_field->field_visibility=...
> > change but in sql_yacc.yy this type of code is use everywhere
>
> Yes :) Many years ago Lex was defined to be current_thd->lex and on some
> platforms current_thd (which is pthread_getspecific) is expensive, so we
> were trying to avoid using it when possible. That's why Lex was saved in
> a local variable.
>
> But now (also, for many years) Lex is YYTHD->Lex, where YYTHD is the
> argument of the MYSQLyyparse function, not pthread_getspecific.
> So there is no need to avoid Lex anymore. Nobody bothered to fix the old
> code, though.
>
> Regards,
> Sergei
> Chief Architect MariaDB
> and
security@mariadb.org