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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp