Hi Varun, I'm still looking at MDEV-8306 code. Sending the input I have so far. There's nothing important but I thought I'd share it now so you can address it while I'm continuing with the review.
propagate_equal_field_for_orderby
Please use "order_by" as in all other functions, and also change "field" to "fields". The name becomes propagate_equal_fields_for_order_by.
void JOIN::propagate_equal_field_for_orderby() { if (!sort_nest_possible) return; ORDER *ord; for (ord= order; ord; ord= ord->next) { if (optimizer_flag(thd, OPTIMIZER_SWITCH_ORDERBY_EQ_PROP) && cond_equal) {
Is there any reason not to check optimizer_switch flag value once at the start of the function?
void JOIN_TAB::find_keys_that_can_achieve_ordering() { if (!join->sort_nest_possible) return;
table->keys_with_ordering.clear_all(); for (uint index= 0; index < table->s->keys; index++) { if (table->keys_in_use_for_query.is_set(index) && test_if_order_by_key(join, join->order, table, index)) table->keys_with_ordering.set_bit(index); } table->keys_with_ordering.intersect(table->keys_in_use_for_order_by); }
Is there any reason to call test_if_order_by_key() and then interset with keys_in_use_for_order_by? Why not just check that bitmap first, like it's done for keys_with_ordering? ... in struct JOIN_TAB:
void find_keys_that_can_achieve_ordering(); bool check_if_index_satisfies_ordering(int index_used);
are "achieving ordering" and "satisfying ordering" the same thing? If yes, it's better to use one term for this.
Item* Item_subselect::transform(THD *thd, Item_transformer transformer, bool transform_subquery, uchar *arg)
We've already discussed this, but I'll mention it here to keep track: this should ON expressions. I would also consider adding an assert that the subquery's joins do not have the query plans, yet.
class Field { ... void statistics_available_via_keys(); void statistics_available_via_stat_tables();
These names need to be better. Function names typically start with a verb. This is especially true for functions that return nothing.
bool Item_func_between::predicate_selectivity_checker(void *arg) bool Item_func_in::predicate_selectivity_checker(void *arg)
These functions need a comment.
bool Item_equal::predicate_selectivity_checker(void *arg) { ... while (it++) { Field *field= it.get_curr_field(); field->is_range_statistics_available();
What is the above? Does the function have some side-effect? This needs to be fixed or at least commented. ... later in the same function:
it.rewind(); Item *item= (it++); SAME_FIELD *same_field_arg= (SAME_FIELD *) arg;
if (same_field_arg->item == NULL) { item->is_resolved_by_same_column(arg); return false; }
This looks very confusing, too. Why are we checking the first element in the Item_equal. Are we assuming it is the constant? But the execution reaches this point even when Item_equal doesn't contain a constant?
bool Item_equal::is_statistics_available()
Maybe, rename this to is_range_statistics_available() to make it clear with what kind of statistics is it?
void add_nest_tables_to_trace(Mat_join_tab_nest_info* nest_info)
rename this to trace_XXX() to be uniform with other tracing functions? e.g. rename to trace_tables_in_sort_nest BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net