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
_______________________________________________
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
