maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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;

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@xxxxxxxxxxx