MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Øystein Grøvlen Date:June 25 2009 1:51pm
Subject:Re: WL#2649
View as plain text  
Bar,

Forgot to mention two things:

1. Since I am not able to understand from the work log how things are
    supposed to work, I have given on verifying that your changes behaves
    correctly with respect default charset vs ASCII.  (Since you did not
    write nor nor approved the worklog, that is not your fault.)

2. When I run the tests on my Solaris desktop, I get the following error
    (similar for the other variants of the test):

CURRENT_TEST: main.ctype_ucs
--- 
/export/home/tmp/oysteing/mysql/mysql-6.0/mysql-test/r/ctype_ucs.result 
    Thu Jun 25 12:10:17 2009
+++ 
/export/home/tmp/oysteing/mysql/mysql-6.0/mysql-test/r/ctype_ucs.reject 
    Thu Jun 25 16:47:41 2009
@@ -1847,7 +1847,7 @@
  drop table t1;
  select hex(concat(acos(0.5)));
  hex(concat(acos(0.5)))
-0031002E0030003400370031003900370035003500310031003900360035003900370036
+0031002E0030003400370031003900370035003500310031003900360035003900370039
  create table t1 as select concat(acos(0.5)) as c1;
  show create table t1;
  Table  Create Table

This issue is probably not related to your changes, so maybe you should 
pick another function than acos to make your test stable across platforms.

See also some in-line responses.

Alexander Barkov wrote:
> 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.

But is this what the worklog is about?  I thought the work log was about 
making sure that string parameter functions returned the right type.  As 
far as I can tell, you have changed the Item classes for the string 
parameter functions, not the Item classes for numeric functions.  Don't 
you need to verify that ADDDATE now returns VARCHAR and not VARBINARY? 
Conversely, why would a string parameter function behave differently 
with respect to return type on one function that returns double than on 
another function that returns double?

> 
> 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.

Great.

> 
> 
>>
>> 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?

Yes, I think that should be OK.

> 
>>
>> 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.

Very good.

-- 
Øystein
Thread
bzr commit into mysql-6.0 branch (bar:2701) WL#2649Alexander Barkov4 Mar
  • Re: bzr commit into mysql-6.0 branch (bar:2701) WL#2649Øystein Grøvlen24 Apr
    • Re: bzr commit into mysql-6.0 branch (bar:2701) WL#2649Alexander Barkov29 Apr
      • Re: bzr commit into mysql-6.0 branch (bar:2701) WL#2649Øystein Grøvlen5 May
        • WL#2649Alexander Barkov19 Jun
          • Re: WL#2649Øystein Grøvlen25 Jun
            • Re: WL#2649Alexander Barkov25 Jun
              • Re: WL#2649Øystein Grøvlen25 Jun
                • Re: WL#2649Peter Gulutzan29 Jun
                  • Re: WL#2649Øystein Grøvlen1 Jul
                    • Re: WL#2649Peter Gulutzan2 Jul
                      • Re: WL#2649Øystein Grøvlen3 Jul
                        • Re: WL#2649Peter Gulutzan15 Jul
                        • Re: WL#2649Alexander Barkov20 Aug
                        • Re: WL#2649Alexander Barkov20 Aug
                          • Re: WL#2649Øystein Grøvlen21 Aug
    • Re: bzr commit into mysql-6.0 branch (bar:2701) WL#2649Alexander Barkov5 May
      • Re: bzr commit into mysql-6.0 branch (bar:2701) WL#2649Alexander Barkov5 May
        • Re: bzr commit into mysql-6.0 branch (bar:2701) WL#2649Øystein Grøvlen6 May
      • Re: bzr commit into mysql-6.0 branch (bar:2701) WL#2649Øystein Grøvlen6 May