maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Varun,

Please more input below:

Input on code structure:

>  SORT_INFO *filesort(THD *thd, TABLE *table, Filesort *filesort...
>  {
>    ...


>  if (!(sort_keys= filesort->make_sortorder(thd, join, first_table_bit)))
>    DBUG_RETURN(NULL);  /* purecov: inspected */

The main effect of this function used to be to create Filesort::sortorder, and
so the name 'make_sortorder' used to make sense. 
Now, it creates a Sort_keys object so the name is counter-intuitive.

.. 
>   uint sort_len= sortlength(thd, sort_keys, &multi_byte_charset,
>                            &allow_packing_for_sortkeys);

Another counter-intuitive name. Maybe this should be a method of sort_keys
object with a different name?

And maybe this call and make_sortorder should be moved together since they're
logically doing the same thing?

...
> Sort_keys*
> Filesort::make_sortorder(THD *thd, JOIN *join, table_map first_table_bit)
> {
...
>   if (!sortorder)
>     sortorder= (SORT_FIELD*) thd->alloc(sizeof(SORT_FIELD) * count);

Using "if(!sortorder)" means the sort order can be already present? Is this
because we're inside a subquery which we are re-executing?


...
>   Sort_keys* sort_keys= new Sort_keys(sort_keys_array);

But then, we create sort_keys every time, and we do it on a MEM_ROOT which
causes a potential O(#rows_examined) memory use. Is my logic correct?

I think we should only call make_sortorder() if this hasn't already been done. 
Any objections to this?

> 
> void Filesort_buffer::sort_buffer(const Sort_param *param, uint count)
> {
> 
>   ...
>   qsort2_cmp cmp_func= param->using_packed_sortkeys() ?
>                        get_packed_keys_compare_ptr() :
>                        get_ptr_compare(size);
> 
>   my_qsort2(m_sort_keys, count, sizeof(uchar*), cmp_func,
>             param->using_packed_sortkeys() ?
>             (void*)param :
>             (void*) &size);

This choose-the-comparison-function logic is duplicated in merge_buffers().

Can we have in one place? I would add appropriate members into class Sort_key
(or Sort_param).


>   if (param->using_packed_addons() || param->using_packed_sortkeys())
>   {
>       /*
>         The last record read is most likely not complete here.
>         We need to loop through all the records, reading the length fields,
>         and then "chop off" the final incomplete record.
>       */

Why change the identation of this block, from correct to invorrect one? 
Please move it back two spaces to the left.


> static uint make_sortkey(Sort_param *param, uchar *to, uchar *ref_pos)
>

This function only seems to access Sort_param members that relate to Sort_keys.
Did you consider making it accept Sort_keys as an argument?

This seems like a sound idea to me: make_sortkey() is only concerned with
making sort keys from original records. This is what Sort_keys class should be
handling. Sort_param on the other hand covers the entire sorting process: 
the buffers, total # of rows, etc.
Please give it a try.

BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog