Hi Yuchen,
> commit c4c4bd8e47e5bfd4d0a650e286ea9daa3e6276f0
> Author: Yuchen Pei <y...@mariadb.com>
> Date: Thu Nov 2 11:55:42 2023 +1100
>
> MDEV-27576 Use reverse index for max/min optimization
>
> We add a field in st_table_ref to indicate that the key used is
> a descending index, which will flip the functions and flags used in
> get_index_max_value() and get_index_min_value(), that allows correct
> optimization for max/min for descending index.
Generally the patch looks good. I have only a few cosmetic comments.
Please rename the test file from mdev_27576.test to something more descriptive
like desc_index_min_max.test (following desc_index_range.test)
Also please rename the table in the test to t1.
> diff --git a/sql/sql_select.h b/sql/sql_select.h
> index e0bbadadc69..45d7527280a 100644
> --- a/sql/sql_select.h
> +++ b/sql/sql_select.h
> @@ -126,6 +126,7 @@ typedef struct st_table_ref
> uint key_parts; ///< num of ...
> uint key_length; ///< length of key_buff
> int key; ///< key no
> + bool reverse; ///< key is reverse
> uchar *key_buff; ///< value to look for with key
> uchar *key_buff2; ///< key_buff+key_length
> store_key **key_copy; //
It's actually not "key is reverse" but "last used key part is reverse"...
also this member is never set for the most common scenario of TABLE_REF use,
which is ref access.
The member is set in a rather trivial logic in find_key_for_maxmin(),
and then it's used in get_index_min_value() and get_index_max_value().
I suggest that we do without this member. get_index_min_value() and
get_index_max_value() can check:
bool reverse=
table->key_info[ref->key].key_part[ref->key_part]->key_part_flag &
HA_REVERSE_SORT
> diff --git a/sql/opt_sum.cc b/sql/opt_sum.cc
> index 794ec40fc66..1f94a4f4dc7 100644
> --- a/sql/opt_sum.cc
> +++ b/sql/opt_sum.cc
> @@ -114,7 +114,8 @@ static int get_index_min_value(TABLE *table, TABLE_REF
> *ref,
> int error;
>
> if (!ref->key_length)
> - error= table->file->ha_index_first(table->record[0]);
> + error= ref->reverse ? table->file->ha_index_last(table->record[0]) :
> + table->file->ha_index_first(table->record[0]);
Please add { ... } as the if-branch is a multi-line statement now.
> else
> {
> /*
> @@ -206,8 +210,12 @@ static int get_index_max_value(TABLE *table, TABLE_REF
> *ref, uint range_fl)
> table->file->ha_index_read_map(table->record[0], ref->key_buff,
>
> make_prev_keypart_map(ref->key_parts),
> range_fl & NEAR_MAX ?
> - HA_READ_BEFORE_KEY :
> - HA_READ_PREFIX_LAST_OR_PREV) :
> + (ref->reverse ? HA_READ_AFTER_KEY :
> + HA_READ_BEFORE_KEY) :
> + (ref->reverse ? HA_READ_KEY_OR_NEXT
> :
> + HA_READ_PREFIX_LAST_OR_PREV)) :
> + ref->reverse ?
> + table->file->ha_index_first(table->record[0]) :
> table->file->ha_index_last(table->record[0]));
Multiple levels of x?y:z statements are hard to read.
Please change to
if (ref->key_length)
{
...
}
else
{
...
}
BR
Sergei
--
Sergei Petrunia, Software Developer
MariaDB Corporation | Blog: http://petrunia.net
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org
