List:Commits« Previous MessageNext Message »
From:He Zhenxing Date:January 20 2011 10:23am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:3513)
Bug#51638
View as plain text  
Sven Sandberg wrote:
> Hi Zhenxing,
> 
> Thanks for your reply. See comments inline.
> 
> He Zhenxing wrote:
> [...]
> >> (S2) I think the "Result" class is not needed, it is not used in an 
> >> object-like manner: we only create one object instance and call one 
> >> method. So it works more like a function. I would therefore suggest to 
> >> simply use functions instead. I think we can use 
> >> bitmap_is_set/bitmap_union/bitmap_set instead of the wrapper functions.
> >>
> > 
> > The class Result and the wrapper functions are there to make the code
> > neat, easier to understand and maintain. Without the class Result, we
> > will have to add some global variables (table, const_bitmap,
> > ordered_bitmap) or passing the parameters around the functions. Using
> > the wrapper function makes the purpose of the code more clear, and also
> > we can easily change there implementation (if we choose something better
> > than bitmap).
> > 
> > And if we will support multi-table, we will have multiple instances of
> > this class.
> 
> OK, fair enough :-)
> 
> [...]
> >> (S4) Even when there is a WHERE pk_col = constant clause, it's possible 
> >> to have more than one matching rows in some cases. For example, every 
> >> number has many string representations:
> >>      CREATE TABLE t1 (a CHAR(10) PRIMARY KEY);
> >>      CREATE TABLE t2 LIKE t1;
> >>      INSERT INTO t2 VALUES (''),('0'),('0.'),('.0'),('0.0'),('00');
> >>      INSERT INTO t1 SELECT * FROM t2 WHERE a = 0 LIMIT 1;
> >> The last statement is actually unsafe, any of the six rows could match. 
> >> I don't know if it would be enough to check that the constant is of the 
> >> same type as the column. It's probably best to ask the runtime team 
> >> about that.
> >>
> > 
> > Right, It looks it's not enough to check that they are the same type, I
> > found the following case in check_simple_equality().
> > 
> >     if (const_item &&
> >         field_item->result_type() == const_item->result_type())
> >     {
> >       if (field_item->result_type() == STRING_RESULT)
> >       {
> >         CHARSET_INFO *cs= ((Field_str*) field_item->field)->charset();
> >         ...
> >         if ((cs != ((Item_func *) item)->compare_collation()) ||
> >             !cs->coll->propagate(cs, 0, 0))
> >           return FALSE;
> >       }
> >     }
> > 
> > I think we should to do the same, for STRINGs, the collation of the
> > field should be used for comparison.
> 
> Sounds good.
> 
> [...]
> >> (S7) A table can be order-deterministic if there is a PK where some 
> >> components are ordered by ORDER BY and some are unique because of WHERE 
> >> col = constant clauses. For example, the following is 
> >> order-deterministic, but the current algorithm marks it as unsafe:
> >>      CREATE TABLE t1 (a INT, b INT, PRIMARY KEY(a, b));
> >>      CREATE TABLE t2 LIKE t1;
> >>      INSERT INTO t2 SELECT * FROM t1 WHERE a = 1 ORDER BY b;
> >> See comments inline.
> >>
> > 
> > Yes, I ignore this for simplicity, I think this use case is not very
> > important, will handle this in the patch for trunk.
> 
> I don't think this is more complicated than the current implementation - 
> see my suggestion for (S6)+(S7) inline in previous email.
> 
> Also, I think there are quite natural cases where this is needed. 
> Imagine for instance a database of course schedules. There is a table 
> where each lecture has one row. There is a PK on (course_id, 
> lecture_time). Then, it seems quite natural to use
> WHERE course_id=X ORDER BY lecture_time LIMIT page_size.
> 

OK, will do that

> >> ==== Other notes ====
> >>
> >> (S8) I think you use the term "order deterministic" to denote that the 
> >> rows are in a deterministic order, in all places except 
> >> is_unique_key_const_ordered. Also, this doesn't check if a key is order 
> >> deterministic, it checks if a table is order deterministic. So I think 
> >> we can rename it to is_table_order_deterministic. See comments inline.
> >>
> > 
> > OK, I think I can remove the function, and move the code to
> > Result::is_ordered();
> 
> Sounds good.
> 
> /Sven
> 


Thread
bzr commit into mysql-5.1-bugteam branch (zhenxing.he:3513) Bug#51638He Zhenxing11 Dec
  • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:3513) Bug#51638Sven Sandberg18 Jan
    • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:3513)Bug#51638He Zhenxing19 Jan
      • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:3513) Bug#51638Sven Sandberg20 Jan
        • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:3513)Bug#51638He Zhenxing20 Jan