Hi Yuchen,
On Thu, Nov 09, 2023 at 11:57:35AM +1100, Yuchen Pei wrote:
> Hi Sergey,
>
> > [... 14 lines elided]
>
> > Generally the patch looks good. I have only a few cosmetic comments.
>
> Thanks for the comments. I've updated my patch and the commit is in my
> latest comment in the ticket. I understand this task has fix version
> 11.5 so no rush.
>
> > Please rename the test file from mdev_27576.test to something more
> > descriptive
> > like desc_index_min_max.test (following desc_index_range.test)
>
> Done.
>
> > Also please rename the table in the test to t1.
>
> Done, but why?
It's a convention to name tables t1,t2 ...
One may argue if it has a lot of merit. At least one of them is that one
known which table names would not conflict with tables used in the test.
> >> 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
>
> I tried this, but it did not work. For example it will not work on
> ...
I still think it could work, I've just made a typo, it should be
table->key_info[ref->key].key_part[ref->key_parts-1].key_part_flag
but it's fine if a local variable is passed around.
Please apply the attached diff fixing coding style.
After that let's create a preview tree and submit the feature for
testing.
BR
Sergei
--
Sergei Petrunia, Software Developer
MariaDB Corporation | Blog: http://petrunia.net
diff --git a/sql/opt_sum.cc b/sql/opt_sum.cc
index 4bbde663907..30739054ab2 100644
--- a/sql/opt_sum.cc
+++ b/sql/opt_sum.cc
@@ -117,7 +117,7 @@ static int get_index_min_value(TABLE *table, TABLE_REF *ref,
if (!ref->key_length)
{
error= reverse ? table->file->ha_index_last(table->record[0]) :
- table->file->ha_index_first(table->record[0]);
+ table->file->ha_index_first(table->record[0]);
}
else
{
@@ -220,10 +220,11 @@ static int get_index_max_value(TABLE *table, TABLE_REF *ref, uint range_fl,
HA_READ_BEFORE_KEY) :
(reverse ? HA_READ_KEY_OR_NEXT :
HA_READ_PREFIX_LAST_OR_PREV));
- } else
+ }
+ else
{
return reverse ? table->file->ha_index_first(table->record[0]) :
- table->file->ha_index_last(table->record[0]);
+ table->file->ha_index_last(table->record[0]);
}
}
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org
