maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

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