Pomelo.EntityFrameworkCore.MySql icon indicating copy to clipboard operation
Pomelo.EntityFrameworkCore.MySql copied to clipboard

Change `Match` extension methods and introduce `IsMatch` ones

Open lauxjpn opened this issue 3 years ago • 5 comments

The Match() methods should return the relevance value of the match in the same way as the MATCH...AGAINST clause does in MySQL.

For convenience, we keep the previous functionality to just return true or false around, but rename those methods to IsMatch(), that just translate to MATCH() ... AGAINST() > 0.0.

Fixes #1679

lauxjpn avatar Jul 09 '22 14:07 lauxjpn

Did you intentionally skip the arguments[3] is SqlParameterExpression parameter case for the new .Match-mapper?

TheConstructor avatar Jul 11 '22 07:07 TheConstructor

Did you intentionally skip the arguments[3] is SqlParameterExpression parameter case for the new .Match-mapper?

~~@TheConstructor I'm not sure which line you are referring to. Just directly comment on the line (using the Files changed tab).~~

Ah, I see now what you mean. Yes, that is intentional. I don't see a simple way for the generated SQL to switch between the 3 available MySqlMatchSearchMode options, ensure the usage of available fulltext indices and make it work for MySQL and MariaDB.

That's why for Match():

/// <param name="searchMode">The mode to performed the search with. Needs to be a constant value or throws otherwise.</param>

lauxjpn avatar Jul 14 '22 20:07 lauxjpn

I guess you could try a CASE WHEN expression, and keep recommending to use a constant value.

Just re-read the comment. I guess using index would be interesting, if used in ORDER BY, and Maria DB-support would also be great, but implementing CASE WHEN shouldn't be worse than no translation.

TheConstructor avatar Jul 15 '22 07:07 TheConstructor

Just re-read the comment. I guess using index would be interesting, if used in ORDER BY, and Maria DB-support would also be great, but implementing CASE WHEN shouldn't be worse than no translation.

The chosen path that EF Core takes (which I think is reasonable), is to provide only performant options by default, because most users will not investigate performance issues and just expect things to work well out-of-the-box.

That provides us with the following options:

  1. Only support a constant value and throw otherwise (current PR).
  2. Make a partially unoptimized implementation (like using CASE) opt-in.
  3. Support only optimized/index using CASE implementations and throw otherwise (i.e. for MariaDB).
  4. Revert to using explicit methods (like MatchBooleanMode()) that kind of force users to explicitly choose a mode.

Which I see as follows:

  1. is simple, but with a higher chance to throw at runtime.
  2. seems too much work to support a specific corner case.
  3. is reasonable, but might be confusing when switching databases.
  4. is what other providers do: more redundant but kind of forces the users hand.

So in my opinion, all options except 2 are probably fine, all having their individual pros and cons.

@TheConstructor Do you think we should implement option 3 instead of option 1?

lauxjpn avatar Jul 16 '22 07:07 lauxjpn

I dislike, that the exceptions are only occuring at runtime, as it is not always trivial to test all possible query variants. The user is already able to implement CASE WHEN, so maybe an explaining exception would be the best approach. 4 would make upgrading more difficult, but would remove this problem completely.

So after your explanation I would probably add a specific exception, that states that the last argument needs to be a constant for now.

Another question, that came to my mind, is, if we should give the user a way to pass a value other than 0.0 to IsMatch.

TheConstructor avatar Jul 16 '22 08:07 TheConstructor

@TheConstructor Sorry, I forget this one is still open! Since we have already published a silver release, this PR will unfortunately not make it into 7.0.x anymore. :disappointed:

lauxjpn avatar Dec 16 '22 16:12 lauxjpn

@lauxjpn so when to expect some movement now? 8.0 sounds like autumn?

TheConstructor avatar Mar 10 '23 18:03 TheConstructor

@TheConstructor This should make it into 8.0.0-beta.2.

Another question, that came to my mind, is, if we should give the user a way to pass a value other than 0.0 to IsMatch.

Since Match() does still exist (in addition to IsMatch()), I think it is simple enough to just write Match(...) > 0.42 for anyone who wants to customize the behavior.

lauxjpn avatar Oct 01 '23 23:10 lauxjpn