Hello Sergei,

Thanks for the review.


On 10/18/23 12:29 AM, Sergei Golubchik wrote:
Hi, Alexander,

The actual code change (replacing strnncollsp with strnncollsp_nchars)
is ok.

But I didn't like the functions you've created.
The *_ft_* family is quite confusing, I would not expect to see
fulltext-specific functions my_compare.h. I would expect to see
there functions which are generic and named by what they're doing, not
by where they should be used.

ha_compare_char_fixed and ha_compare_char_varying are better. Names are
clearer and it's kind of understandable what they do.

I suggest you remove *_ft_* functions. fulltext code should, I guess,
just use charset_info->coll->strnncoll() everywhere. Not strnncollsp(),
because words normally don't have trailing spaces, and if some
custom pluggable parser returns words with trailing spaces, they're
likely a part of the word and should be compared too.

I removed *_ft_* functions. Please find a new patch here:


https://github.com/MariaDB/server/commit/62078f55b832402a2e2c6982349c02fc4c15328d

As agreed on slack, this patch does not change strnncollsp() to strnncoll(), that will be done separately when needed.

Thanks.


Regards,
Sergei
Chief Architect, MariaDB Server
and secur...@mariadb.org

On Oct 17, Alexander Barkov wrote:
revision-id: 56fa1da9c67 (mariadb-10.4.28-90-g56fa1da9c67)
parent(s): ed2adc8c6f9
author: Alexander Barkov
committer: Alexander Barkov
timestamp: 2023-04-07 14:54:17 +0400
message:

MDEV-30048 Prefix keys for CHAR work differently for MyISAM vs InnoDB

Also fixes: MDEV-30050 Inconsistent results of DISTINCT with NOPAD

Problem:

Key segments for CHAR columns where compared using strnncollsp()
for engines MyISAM and Aria.

This did not work correct in case if the engine applyied trailing
space compression.

Fix:

Replacing ha_compare_text() with a number of new functions:

- ha_compare_ft_text_full()
- ha_compare_ft_text_prefix()
- ha_compare_ft_text()
- ha_compare_char_varying()
- ha_compare_char_fixed()

The code branch corresponding to comparison of CHAR column keys
(HA_KEYTYPE_TEXT segment type) now uses ha_compare_char_fixed()
which calls strnncollsp_nchars().

For the rest of the code:
- comparison of VARCHAR/TEXT column keys
   (HA_KEYTYPE_VARTEXT1, HA_KEYTYPE_VARTEXT2 segments types)
- comparison in the fulltext code
this patch does not change the behaviour.
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org

Reply via email to