List:Commits« Previous MessageNext Message »
From:Martin Hansson Date:April 20 2010 9:40am
Subject:Re: bzr commit into mysql-next-mr branch (gshchepa:3128) Bug#30584
Bug#36569
View as plain text  
Gleb Shchepa skrev 2010-04-16 11.37:
> Hello Martin,
>
> Thank you for a big review. Please read my answers below.

Well a big patch deserves a big review, right? :-) We are close now. The 
only thing I have to insist on is some change to the use of 
wrap_field_or_item class. See below.

> ...
>
>>
>>> + QUICK_SELECT_I *make_reverse() { return NULL; } // is already
>>> reverse sorted
>>
>> I'm confused by the contract of this interface. If make_reverse() is
>> called for a QUICK_SELECT_DESC, why don't you either return 'this' or
>> reverse it back again? Does the other code rely on NULL being returned
>> if you try to call this particular method?
>>
>> If you intend that nobody should ever call
>> QUICK_SELECT_DESC::make_reverse, then add a DBUG_ASSERT(FALSE).
>>
>
> Fixed (replaced with "this" pointer).

Great. I hope you agree with me that this is more natural. :-) You might 
want to say in the comment something to the effect of. "Returns a 
QUICK_SELECT with reverse order of to the index."

>
>>> + QUICK_SELECT_I *set_quick(QUICK_SELECT_I *new_quick)
>>> + {
>>> + delete quick;
>>> + return quick= new_quick;
>>> + }
>>
>> What is the purpose of returning the argument? Normally a set method
>> returns void. This will surprise programmers and programmers don't
>> like being surprised. ;-)
>
> This set method has same semantic as operator=(): assign and return the
> value.
> In this case we can write something like if(a.set_quick(b.get_quick())
> instead of:
> QUICK_SELECT_I *tmp;
> if ((tmp= b.get_quick())
> a.set_quick(b);
>
> If you insist on that, I'll change it.

If you were overloading operator=, this would be the right thing to do. 
But accessor methods aren't assignment. I'm not saying your way of doing 
it is bad, it's just that it will make people surprised. set_xxx methods 
returning void is just a convention.

Let's make a quick survey what is the most common:

martin:~/bzr/mysql-5.1-bugteam/sql>egrep 
"(char|int|double|float|bool|[A-Za-z_]+\*)\ +set_" *.{cc,h} | wc -l
       91
martin:~/bzr/mysql-5.1-bugteam/sql>grep void\ *set_ *.{cc,h} | wc -l
      231


Since we don't have exceptions, I can live with that fact that typical 
void functions may return true in case of errors, but not otherwise. In 
fact, returning bool from a set_xxx function seems to be the most common 
case:

martin:~/bzr/mysql-5.1-bugteam/sql>egrep "bool\ +set_" *.{cc,h} | wc -l
       53

If I still haven't convinced you, do as you please. :-)



>
>>
>>>
>>> /**
>>> @@ -55,10 +57,11 @@ static int rr_index(READ_RECORD *info);
>>> @param print_error If true, call table->file->print_error() if an
> error
>>> occurs (except for end-of-records error)
>>> @param idx index to scan
>>> + @param reverse Scan in the reverse direction
>>> */
>>
>> I think you need to update the comment that says 'in forward direction'
>> above. It is very dangerous to change a function's behavior without
>> changing its documentation.
>
> I disagree, please see:
>
> info->read_record= reverse ? rr_index_last : rr_index_first
>
> I.e. "reverse == TRUE" means exactly what you read in the commentary:
> we scan in the reverse direction.
>

I meant this comment (above init_read_record_idx):

"Initialize READ_RECORD structure to perform full index scan (in forward 
direction) using read_record.read_record() interface."

You have now modified this function to also initialize the READ_RECORD 
structure to scan in reverse direction. So IMHO the comment should be

"Initialize READ_RECORD structure to perform full index scan in desired 
direction using read_record.read_record() interface."

>> Please move the declaration of reverse and need sort to directly above
>> the following line.
>>
>>> + usable_index= get_index_for_order(order, table, select, limit,
>>> +&need_sort,&reverse);
>>> + if (need_sort)
>>> {
>>> + DBUG_ASSERT(usable_index == MAX_KEY);
>>> table->sort.io_cache= (IO_CACHE *) my_malloc(sizeof(IO_CACHE),
>>> MYF(MY_FAE | MY_ZEROFILL));
>
> I can't do it for "reverse": it is used outside the block.
> Done for need_sort.

Right, my mistake, sorry.

> Added a description:
>
> This function is a limited version of remove_const() for use
> with non-JOIN statements (i.e. single-table UPDATE and DELETE).

Thank you, that will save other people's time. :-)
>
>>
>>> +
>>> static int
>>> return_zero_rows(JOIN *join, select_result *result,TABLE_LIST *tables,
>>> List<Item> &fields, bool send_row, ulonglong select_options,
>>> @@ -9510,12 +9549,38 @@ test_if_equality_guarantees_uniqueness(I
>>> l->collation.collation == r->collation.collation)));
>>> }
>>>
>>> -/**
>>> - Return TRUE if the item is a const value in all the WHERE clause.
>>> +/*
>>> + Generic wrapper to pass Field* or Item* to
>>> const_expression_in_where().
>>> */
>>> +class wrap_field_or_item {
>>
>> You don't need this class. Nor do you need const_[field|item]_in_where()
>> or modify const_expression_in_where. There is a great wrapper class to
>> do this job for you. It's called Item_field :-)
>>
>> Whenever you need to call const_expression_in_where with a Field, just do
>>
>> const_expression_in_where(where, new Item_field(field), &dummy)
>>
>> You can even allocate it on the stack, but then you need to manually
>> remove it from the thd->free_list. Item's add themselves there during
>> the Item cosntructor, which is completely boneheaded. It's fine to let
>> objects add themselves to a free list but it should be done in the
>> 'new' operator.
>>
>> In fact, there is one place where an Item is allocated on the stack in
>> log_event.cc. How they pull it off I can't understand.
>>
>> I'm attaching a patch to fix this to this email.
>
> I can't agree. IMO new Item_field to access field item (considering
> its free_list manipulation) is a surplus there. OTOH if you insist on
> that, I'll replace this class with "new Item_field".

If you mean that the class Item_field is bloated, I agree with you. But 
the original purpose of this class is precisely what your new class 
does: Adapter the Field interface to the Item interface. IMHO it will 
make the code more complicated if we start creating new classes just 
because the old classes are doing too much work that isn't really their 
core duty.

I can understand if you find my patch too risky to include here, though. 
If you really want to keep your approach, let's discuss it. I think I 
can help you find a way to do it with less than one new class and two 
new functions.

For instance you can remove the 'explicit' keyword on 
wrap_field_or_item::wrap_field_or_item and then you don't need the 
functions const_[item|field]_in_where. I know Evgeny doesn't agree but I 
think we can expect our developers to understand C++.

Another option is to create a class e.g. Comparable that Item and Field 
can inherit. This class will take no space since it will only have your 
non-virtual method eq(). Although I would encourage you to name it 
something better, like 'equals'.

You could use templates as well, but I wouldn't recommend it.

If you still want to keep your current approach, please note:

- Class names should be nouns, not verbs.
- Class names begin with a capital according to coding standards.

Hence: Expression_wrapper, Field_or_item, Field_or_item_wrapper would be 
acceptable names.


>>
>> - You keep passing the arguments ORDER* order, TABLE* table and int key
>> (or sometimes called idx) together through the functions
>> get_index_for_order, test_if_cheaper_ordering, test_if_order_by_key etc.
>> Perhaps it would make sense to create some class to hold them
>> instead? If you still want to keep passing them around, please at least
>> pass them in the same order every time.
>
> Fixed (reordered parameters in a same order).
>
> I had a try to invent a new structure, say, ORDER_INFO of ORDER*, TABLE*
> and
> limit values. IMO it doesn't help to make the code nicer :-(

Ok, so at least you tried it and discarded the idea. One can only do so 
much. :-)


>
>>
>>> ...
>>> - uint used_index= MAX_KEY, dup_key_found;
>>> + uint used_index, dup_key_found;
>>> bool need_sort= TRUE;
>>> + bool reverse= FALSE;
>>
>> Please declare need_sort and reverse when they are used, not at the
>> beginning of the function.
>
> In mysql_update()? I can't: there is a goto and a label around this place.

This should be a lesson to all of us: Don't use goto. No, really. Dont. 
Use. Goto. Ever.

>
>>
>>> ...
>>> +
>>> +bool is_simple_order(ORDER *order)
>>> +{
>>> + for (ORDER *ord= order; ord; ord= ord->next)
>>> + {
>>> + if (ord->item[0]->real_item()->type() != Item::FIELD_ITEM)
>>> + return FALSE;
>>> + }
>>> + return TRUE;
>>> +}
>>
>> This would be better off as a method (member function) of ORDER, IMHO.
>
> It was a method in some previous commit, but Evgeny complained about that:
> this function deal with ORDER _list_, not an ORDER list item.

Ok, keep it. This is why intrusive pointers should never ever be used. 
There is no way to tell the difference between a list of ORDER's and an 
ORDER. I really hope we can get rid of all intrusive pointer some day. 
At least in the optimizer.

Best Regards

Martin
Thread
bzr commit into mysql-next-mr branch (gshchepa:3128) Bug#30584Bug#36569Gleb Shchepa31 Mar
  • Re: bzr commit into mysql-next-mr branch (gshchepa:3128) Bug#30584Bug#36569Martin Hansson9 Apr
    • Re: bzr commit into mysql-next-mr branch (gshchepa:3128) Bug#30584Bug#36569Gleb Shchepa16 Apr
      • Re: bzr commit into mysql-next-mr branch (gshchepa:3128) Bug#30584Bug#36569Martin Hansson20 Apr
        • Re: bzr commit into mysql-next-mr branch (gshchepa:3128) Bug#30584Bug#36569Gleb Shchepa20 Apr