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