Hi Yuchen,
commit c4c4bd8e47e5bfd4d0a650e286ea9daa3e6276f0 Author: Yuchen Pei <ycp@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