Fixed #29251 -- Added CharField converter to the MySQL backend. #9816

felixxm · 2018-03-22T18:22:50Z

https://code.djangoproject.com/ticket/29251

I'm not convinced that this is the best solution, but I haven't found a better way to solve this issue.

ngnpope · 2018-03-22T19:34:32Z

django/db/backends/mysql/operations.py

+    def convert_charfield_value(self, value, expression, connection):
+        if isinstance(value, bytes):
+            return force_text(value)
+        return value


I guess this does follow other existing methods, but:

return force_text(value) if isinstance(value, bytes) else value

Thanks. I updated this line.

timgraham

Is force_text() needed as opposed to value.decode()? If so, can you add a test to demonstrate?

timgraham · 2018-03-23T01:27:07Z

tests/db_functions/tests.py

+    def test_funct_string_combination(self):
+        Author.objects.create(name='Rhonda', alias='john_smith')
+        authors = Author.objects.annotate(filled=LPad('name', Length('alias'), output_field=CharField()))
+        self.assertQuerysetEqual(authors, ['    Rhonda'], lambda a: a.filled, ordered=False)


I wonder why ordered=False with a result of length 1.

timgraham · 2018-03-23T01:27:08Z

tests/db_functions/tests.py

@@ -671,3 +671,8 @@ def test_function_as_filter(self):
            Author.objects.exclude(alias=Upper(V('smithj'))),
            ['Rhonda'], lambda x: x.name
        )
+
+    def test_funct_string_combination(self):


funct -> func -- I think? or "function"

Typo 🤦‍♂️ Thanks.

felixxm · 2018-03-23T12:09:34Z

@timgraham Thanks for the review. You're right decode() is enough.

ngnpope · 2018-03-23T12:19:29Z

I note that convert_textfield_value() below is using force_text().
Perhaps that can be resolved in a follow up commit if .decode() is enough there too?

timgraham · 2018-03-23T12:47:40Z

Yes, there's a ticket to audit force_text() usage.

timgraham · 2018-03-23T13:05:31Z

Then non-ASCII character case doesn't work on Oracle (leading spaces are missing).

felixxm · 2018-03-23T13:41:38Z

LPad/Rpad doesn't work properly with multibyte characters sets on Oracle. I found the third issue in a row 😄

felixxm · 2018-03-23T16:56:01Z

@timgraham I checked and IMO there doesn't exist a good workaround for LPad/RPad with multibyte characters on Oracle. None of them works in all cases. According Oracle documentation:

... In most character sets, this is also the number of characters in the return value. However, in some multibyte character sets, the display length of a character string can differ from the number of characters in the string.

I think it's fine to add a note in documentation about this restriction.

timgraham · 2018-03-23T17:15:27Z

Sure, if you think is deserves a mention. We can't document everything about each database. :-)

felixxm · 2018-03-23T17:36:41Z

You're right, it's impossible to describe all Oracle's caveats 😉 I've left a note in the ticket 28643, this should be enough. I updated tests, it works on Oracle.

claudep · 2018-03-23T17:37:34Z

Would it be worth benchmarking the speed overhead of this addition?

mysqlclient returns bytes instead of string in some edge cases. Thanks Tim Graham and Nick Pope for reviews.

felixxm · 2018-03-27T18:44:45Z

@claudep I wrote simple benchmark with djangobench:

Running benchmarks: mysql_char_converter
Control: Django 2.1.dev20180326173423 (in git branch master)
Experiment: Django 2.1.dev20180327171917 (in git branch issue-29251)

Running 'mysql_char_converter' benchmark ...
Min: 0.038486 -> 0.046070: 1.1970x slower
Avg: 0.043529 -> 0.051424: 1.1814x slower
Significant (t=-10.273530)
Stddev: 0.00555 -> 0.00532: 1.0428x smaller (N = 100)

Control: Django 2.1.dev20180326173423 (in git branch master)
Experiment: Django 2.1.dev20180327171917 (in git branch issue-29251)

Running 'mysql_char_converter' benchmark ...
Min: 0.034216 -> 0.042022: 1.2281x slower
Avg: 0.038185 -> 0.045879: 1.2015x slower
Significant (t=-14.931398)
Stddev: 0.00528 -> 0.00502: 1.0528x smaller (N = 200)

claudep · 2018-03-27T19:33:20Z

Oh thanks! So does that mean we are ~20% slower with the patch? (sorry, I'm not so great at stats)

felixxm · 2018-03-28T20:34:02Z

I think so, but even if convert_charfield_value() doesn't check or change anything I still get about ~11% , i.e. with

def convert_charfield_value(self, value, expression, connection):
    return value

the result is

Running benchmarks: mysql_char_converter
Control: Django 2.1.dev20180326173423 (in git branch master)
Experiment: Django 2.1.dev20180328202753 (in git branch issue-29251)

Running 'mysql_char_converter' benchmark ...
Min: 0.075977 -> 0.085019: 1.1190x slower
Avg: 0.086817 -> 0.096318: 1.1094x slower
Significant (t=-8.154523)
Stddev: 0.00763 -> 0.00881: 1.1550x larger (N = 100)

Control: Django 2.1.dev20180326173423 (in git branch master)
Experiment: Django 2.1.dev20180328202753 (in git branch issue-29251)

Running 'mysql_char_converter' benchmark ...
Min: 0.080404 -> 0.091563: 1.1388x slower
Avg: 0.091934 -> 0.102306: 1.1128x slower
Significant (t=-12.184438)
Stddev: 0.00919 -> 0.00777: 1.1821x smaller (N = 200)

claudep · 2018-03-29T06:30:59Z

That's probably the overhead cost of adding converters (function calls et al). Then I think we should find another way of solving this issue. Slowing the whole stack just to fix this edge case is not very sound IMHO.

felixxm · 2018-03-29T17:34:18Z

I've prepared alternative PR #9835 with conversion only in affected database functions.

felixxm · 2018-03-31T09:51:46Z

Closing due to the alternative approach in #9835.

ngnpope reviewed Mar 22, 2018

View reviewed changes

felixxm force-pushed the issue-29251 branch from 543fd26 to 5ff5590 Compare March 22, 2018 19:58

timgraham reviewed Mar 23, 2018

View reviewed changes

felixxm force-pushed the issue-29251 branch from 5ff5590 to acd86db Compare March 23, 2018 12:07

felixxm force-pushed the issue-29251 branch from acd86db to 50f09e4 Compare March 23, 2018 17:30

Fixed #29251 -- Added CharField converter to the MySQL backend.

6678af8

mysqlclient returns bytes instead of string in some edge cases. Thanks Tim Graham and Nick Pope for reviews.

felixxm force-pushed the issue-29251 branch from 50f09e4 to 6678af8 Compare March 27, 2018 17:20

felixxm closed this Mar 31, 2018

felixxm deleted the issue-29251 branch March 31, 2018 09:52

felixxm mentioned this pull request Jan 28, 2022

Fixed #28949 -- Fixed index name truncation for multibyte characters. #15273

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #29251 -- Added CharField converter to the MySQL backend. #9816

Fixed #29251 -- Added CharField converter to the MySQL backend. #9816

felixxm commented Mar 22, 2018

ngnpope Mar 22, 2018

felixxm Mar 22, 2018

timgraham left a comment

timgraham Mar 23, 2018

timgraham Mar 23, 2018

felixxm Mar 23, 2018

felixxm commented Mar 23, 2018

ngnpope commented Mar 23, 2018 •

edited

timgraham commented Mar 23, 2018

timgraham commented Mar 23, 2018

felixxm commented Mar 23, 2018

felixxm commented Mar 23, 2018 •

edited

timgraham commented Mar 23, 2018

felixxm commented Mar 23, 2018

claudep commented Mar 23, 2018

felixxm commented Mar 27, 2018

claudep commented Mar 27, 2018

felixxm commented Mar 28, 2018

claudep commented Mar 29, 2018

felixxm commented Mar 29, 2018

felixxm commented Mar 31, 2018

Fixed #29251 -- Added CharField converter to the MySQL backend. #9816

Fixed #29251 -- Added CharField converter to the MySQL backend. #9816

Conversation

felixxm commented Mar 22, 2018

ngnpope Mar 22, 2018

Choose a reason for hiding this comment

felixxm Mar 22, 2018

Choose a reason for hiding this comment

timgraham left a comment

Choose a reason for hiding this comment

timgraham Mar 23, 2018

Choose a reason for hiding this comment

timgraham Mar 23, 2018

Choose a reason for hiding this comment

felixxm Mar 23, 2018

Choose a reason for hiding this comment

felixxm commented Mar 23, 2018

ngnpope commented Mar 23, 2018 • edited

timgraham commented Mar 23, 2018

timgraham commented Mar 23, 2018

felixxm commented Mar 23, 2018

felixxm commented Mar 23, 2018 • edited

timgraham commented Mar 23, 2018

felixxm commented Mar 23, 2018

claudep commented Mar 23, 2018

felixxm commented Mar 27, 2018

claudep commented Mar 27, 2018

felixxm commented Mar 28, 2018

claudep commented Mar 29, 2018

felixxm commented Mar 29, 2018

felixxm commented Mar 31, 2018

ngnpope commented Mar 23, 2018 •

edited

felixxm commented Mar 23, 2018 •

edited