Apply string interpolation in mogrify if args is not None

[pmishchenko-ua]

Currently the behavior of execute depends on whether query arguments are non-empty, even when the result set is the same. See test_mogrify_val_with_percent as an example - without the change, the query with an empty parameters list produces incorrect result.

[kesmit13]

I'm going to be honest, I'm not sure this is a good idea. Our driver is based on PyMySQL and is compatible with its behaviors. This change would be a breaking change for any code that assumes those existing behaviors.

[kesmit13]

I double checked and both PyMySQL and MySQLdb clients work the same way ours does. This way of doing parameters is also consistent with the way that Python itself works when printing strings with substitutions using the % operator (although this way of doing substitutions isn't used as much anymore). I think making a change of this magnitude would cause more problems than it solves.

[pmishchenko-ua]

@kesmit13 yeah I also checked that that PyMySQL works this way. I've encountered this problem when developing SingleStore backend for Django - certain query templates with % are used, and they can be a part of a query with or without arguments, and due to different behavior, it's impossible to create a template that would work in both cases. But I also feel like it would break more queries than fix - something like SHOW TABLES LIKE '%pattern%' can be often used "as is" with no parameters. My goal is to get your idea on what would be the best way to get new behavior working somewhere, I think we agree that making it default is a bad option. I can think of two options

1. Introduce a connection string parameter that would trigger new behavior
2. Create a custom release (from a fork or a separate branch)
   My initial thought was 2, but from the maintenance perspective 1 seems to be much better. What should be the way to proceed, what do you think?

[kesmit13]

@pmishchenko-ua I wonder how PyMySQL / mysqlclient work with Django if they work the same way? I'd like to avoid an option if possible, but I would rather do that than create a whole fork just for that one difference.

[pmishchenko-ua]

@kesmit13 good question, I've double checked with Django, it works ot top of mysqlclient. It turns out that the key difference to our implementation is truthiness check vs "not None" check, so this is probably the fix. It won't break queries that don't pass args while allowing query templates with %% to work consistently.

I've updated PR description to reflect the actual fix.

[kesmit13]

@pmishchenko-ua I'm going to have to go down memory lane to try to remember why I took that out in the first place. I do remember removing that. I don't recall if it was for SQLAlchemy or Ibis or something else... I believe the reason was that we support both positional and dictionary style substitution parameters and some package was sending an empty dict instead of None.

[kesmit13]

@pmishchenko-ua It's possible that this was an artifact of when the SingleStoreDB client was a frontend to many other MySQL clients. At some point, we refactored and just made it a frontend for PyMySQL and the Data API, and dropped all of the others. I'll have to run some tests in SQLAlchemy and Ibis just to see if there is any affect there though.

[kesmit13]

@pmishchenko-ua After thinking about this some more, I think a connection option might be the best way to go. I'm pretty leery of changing something that every query goes through. Odds are it would be fine, but there may be some customer with existing code that depends on the current behavior. I'd rather play it safe.

[kesmit13]

These lines of code appear in multiple places. It would be better to put them in a function in the singlestoredb/utils/mogrify.py file and use the function in the if block that needs it.