List:Commits« Previous MessageNext Message »
From:Kristian Nielsen Date:February 4 2008 2:32pm
Subject:Re: bk commit into 5.1 tree (mattiasj:1.2516) BUG#33379
View as plain text  
Mattias Jonsson <mattiasj@stripped> writes:

> Thanks for the reply (Is it OK if I set you as a reviewer?)

Yes, ok, but then you need another reviewer I think as I'm not all that
familiar with this code, maybe set me as first reviewer, and when I'm done you
can ask serg to be second reviewer.

> Is it OK to just use the returned longlong variable "value" with
> length "sizeof(longlong)"? (it will use more bytes in the calculation
> of the hash than needed, but it will allways be correct)

Hm. I'm thinking... I believe MySQL file formats are supposed to be
stored in a system-independent way, right?

And the hash() determines which partition a row goes into, right?

So when moving data files across systems, we must make sure that the hash
value stays constant, so that rows do not move to a different partition.

So the hash() function must be independent of endian. So you cannot just use
the ulonglong value.

Maybe something like this (untested)?

   CHARSET_INFO *cs= &my_charset_bin;
   uint len= bytes_in_rec + (bit_len ? 1 : 0);
   uchar tmp[8];
   longlong value= Field_bit::val_int();
   mi_int8store(tmp, value);
   cs->coll->hash_sort(cs, tmp, len, nr, nr2);

> And a side note:
> In Field_bit::val_int() the default in the switch, should that not
> first calculate the length and use it (or just use ptr directly), does
> it not calculate that eight times now since it is a macro?

I agree this would be cleaner. But I also think the compiler common
subexpression elimination will normally be able to avoid extra calculations in
this case.

 - Kristian.

bk commit into 5.1 tree (mattiasj:1.2516) BUG#33379mattiasj4 Feb
  • Re: bk commit into 5.1 tree (mattiasj:1.2516) BUG#33379Kristian Nielsen4 Feb
    • Re: bk commit into 5.1 tree (mattiasj:1.2516) BUG#33379Mattias Jonsson4 Feb
      • Re: bk commit into 5.1 tree (mattiasj:1.2516) BUG#33379Kristian Nielsen4 Feb