Re: WL#2649

[Alexander Barkov]

Hi Øystein,

Thanks for review, please find my comments inline:

Øystein Grøvlen wrote:
> Hi Bar,
> 
> Thanks for the new patch.  I think the changes you have made are good.
> I will send some details comments in reply to the commit mail.  The
> main issues I have now is:
> 
>  - fix_charset(), see below for a discussion

answered below

> 
>  - Testing: Why are all the tests based on concat?  I would have
>    expected to see tests for all the functions that are listed in the
>    "Proofs" section of the worklog.  Some are tested, but wrapped in
>    concat.  Some, I do not find tests for at all.  And are all tests
>    for concat really necessary?  What difference do expect to find
>    between concat(acos(0.5)) and concat(asin(0.5)).  Both basically
>    tests concat of a double.

It does not matter which function to use actually.
What we test that acos() and asin() and the other *numeric* functions
work fine in *string* context.

Any other function with string parameter can be used
instead of CONCAT().

I don't think that we really need to test absolutely
all numeric functions in combination with all string-parameter
functions.
However, adding a few other string-parameter functions
as an example is a very nice idea. Will add in the next patch.


> 
> Alexander Barkov wrote:
> ...
>  > Note, there are now a lot of pieces of code like this:
>  >
>  >     max_length= 6;
>  >     fix_charset();
>  >
>  >
>  > I still suggest to introduce a new method:
>  >
>  >   fix_length_and_charset(6);
>  >
>  > which will set both length and charset in the simple cases like this.
>  > I.e. the same action what set_char_length() did in the previous version,
>  > but with a better name :)
> 
> I agree that the above is not very good.  I do not think the major
> problem is the repetition of code itself, but the following:
> 
> 1. The above code assumes that before fix_charset() is called, the
>    current charset has mbmaxlen==1.  Even if that is always the case
>    today, it would be good if such dependencies could be avoided.
>    This is kind of the same objection that I had earlier with
>    the unit of max_length changing from #chars to #bytes in the
>    middle of the code.
> 
> 2. This style kind of highlights the default case.  That is,
>    fix_charset() is only called when the default charset is to be
>    used.  Expections from this rule, is often less visible (since it
>    sometimes is achieved by not calling fix_charset).  I think it
>    would be good if the exceptions from the rule were more explicit.
> 
> I guess the reason you cannot set charset in the constructor is that
> the default charset may change before you do fix_fields.  Am I right?

Yes, you're right.

> 
> I wonder whether the best would be if fix_fields set charset to
> default_charset in the beginning, and then those classes that wanted
> something else could set that explicitly in its fix_length_and_dec().
> (This could be done by having a non-virtual "fix_fields" that did this
> before calling the existing virtual fix_fields, which had to be
> renamed.)  Then, max_length would always represent #bytes, and would
> have to be set through a set_char_length method that always to charset
> into account.
> 
> Alternatively, I am OK with a fix_length_and_charset, but I think it
> would be best if charset is explicit
> 
>    fix_length_and_charset(6, default_charset());
> 
> and that fix_length_and_charset should always be used.  That is, no
> direct setting of max_length and charset.  Would that work?

Yes, I think it will work fine.

Do you think if it's ok to change only the Items affected by this WL,
leaving the non-affected Items to set max_length explicitly?

> 
> I think fix_length_and_charset would then look something like this:
> 
> void fix_length_and_charset(uint32 char_length, CHARSET_INFO *cs)
> {
>   max_length= length * cs->mbmaxlen;
>   collation.collation= cs;
> }
> 
> That is, no dependence on the previous setting.
> 
> What do you think?

I agree, no dependence on the previous setting is safer.

Will send a new patch soon.

> 
> 
> -- 
> Øystein