maria-developers team mailing list archive
Mailing list archive
Re: MDEV-21580: Allow packed sort keys in sort buffer, part #3
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,
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?
> 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() :
> 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
> 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.
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog