bpo-34041: Added deterministic parameter to sqlite3.Connection.create_function(). #8086

sir-sigurd · 2018-07-04T10:06:00Z

https://bugs.python.org/issue34041

…te_function().

vstinner · 2018-07-04T10:13:42Z

Modules/_sqlite/connection.c

@@ -808,26 +808,43 @@ static void _pysqlite_drop_unused_cursor_references(pysqlite_Connection* self)
    Py_SETREF(self->cursors, new_list);
 }

+#ifndef SQLITE_DETERMINISTIC
+#define SQLITE_DETERMINISTIC 0x800
+#endif


Why do you have to define the constant in Python? Does it mean that SQLite doesn't support the flag? I would prefer to raise an exception, rather than using an hardcoded constant.

Do you think an exception should raised if Python is compiled with SQLite version that doesn't support this feature but is runned with SQLite version that supports it?

What happens if you pass 0x800 to a SQLite which doesn't know/support this flag? I expect an error.

I checked it with SQLite 3.8.2 (the most ancient dockerized version I found). Function is created, but it's not marked as deterministic, so optimizations are not used, it's reflected in the added test.

IMHO it doesn't make sense to raise exception here because if deterministic is requested, but created function is not marked as deterministic this only affects performance, but indeed, flag should not be passed if it's unsupported as it was working properly only by accident.

vstinner · 2018-07-04T10:14:16Z

Modules/_sqlite/connection.c

-    if (!PyArg_ParseTupleAndKeywords(args, kwargs, "siO", kwlist,
-                                     &name, &narg, &func))
+    if (!PyArg_ParseTupleAndKeywords(args, kwargs, "siO|p", kwlist,
+                                     &name, &narg, &func, &deterministic))


Maybe it would be safer to use a keyword only parameter, to prevent bugs?

bedevere-bot · 2018-07-04T10:14:25Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner · 2018-07-04T14:10:33Z

Modules/_sqlite/connection.c

    {
        return NULL;
    }

-    rc = sqlite3_create_function(self->db, name, narg, SQLITE_UTF8, (void*)func, _pysqlite_func_callback, NULL, NULL);
+    if (deterministic && sqlite3_libversion_number() >= 3008003) {


Hum, if the flag only has an impact on performance, IHMO it's ok to ignore it if SQLite doesn't support it. But I would prefer to use "#ifndef SQLITE_DETERMINISTIC" for this test. Something like:

#ifndef SQLITE_DETERMINISTIC
if (deterministic) eTextRep |= SQLITE_DETERMINISTIC;
#endif

If Python is built on old SQLite: no luck, you will never get deterministic functions. If you build Python on new SQLite but run old SQLite: SQLITE_DETERMINISTIC is ignored, it's fine. If you have new versions in both cases: you're lucky, and you get the optimization :-)

I carefully reread SQLite docs and it appears that non-deterministic functions can't be used in indexes, so it seems that exception should be raised after all.

serhiy-storchaka · 2018-07-04T14:25:16Z

Doc/library/sqlite3.rst

@@ -337,17 +337,23 @@ Connection Objects
      :meth:`~Cursor.executescript` method with the given *sql_script*, and
      returns the cursor.

-   .. method:: create_function(name, num_params, func)
+   .. method:: create_function(name, num_params, func, deterministic=False)


I would make the new boolean parameter a keyword-only.

Yep, I made the same proposal :-) At the C level, it means using "$" instead of "|" before the parameter. Parameters after "$" are keyword-only parameters.

sir-sigurd · 2018-07-04T16:57:41Z

I have made the requested changes; please review again

bedevere-bot · 2018-07-04T16:57:43Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

berkerpeksag · 2018-07-06T08:15:26Z

Doc/library/sqlite3.rst


      The function can return any of the types supported by SQLite: bytes, str, int,
      float and ``None``.

+      .. versionchanged:: 3.8
+        The *deterministic* parameter was added.


Nit: Indentation is off here:

.. versionchanged:: 3.8 The *deterministic* parameter was added.

berkerpeksag · 2018-07-06T08:19:02Z

Misc/NEWS.d/next/Library/2018-07-04-21-24-58.bpo-34041.aKk8t5.rst

@@ -0,0 +1,2 @@
+Add the parameter *deterministic* to the
+sqlite3.Connection.create_function().


It would be better if we can link to the create_function() documentation:

.. untested :meth:`sqlite3.Connection.create_function`

It seems like we are missing "method" here:

the sqlite3.Connection.create_function() -> the sqlite3.Connection.create_function() method

And please add "Patch by Sergey Fedoseev.".

berkerpeksag · 2018-07-06T08:23:46Z

Doc/library/sqlite3.rst

+      called as the SQL function. If *deterministic* is true, the created function
+      is marked as `deterministic <https://sqlite.org/deterministic.html>`_, which
+      allows SQLite to perform additional optimizations. This flag is supported by
+      SQLite 3.8.3 or higher and has no effect on older versions.


Could you please mention that NotSupportedError will be raised as well?

berkerpeksag · 2018-07-06T08:24:34Z

Modules/_sqlite/connection.c


    PyObject* func;
    char* name;
    int narg;
    int rc;
+    int deterministic = 0;
+    int eTextRep = SQLITE_UTF8;


Style nit: We usually don't use camelCase convention in Python codebase.

berkerpeksag · 2018-07-06T08:30:54Z

Lib/sqlite3/test/userfunctions.py

+    @unittest.skipIf(sqlite.sqlite_version_info >= (3, 8, 3), "SQLite < 3.8.3 needed")
+    def CheckFuncDeterministicNotSupported(self):
+        with self.assertRaises(sqlite.NotSupportedError):
+            self.con.create_function("deterministic", 0, int, deterministic=True)


Could you also add a simple test to make sure that deterministic is a keyword-only argument?

self.con.create_function("deterministic", 0, int, True)

fixed in the following test

berkerpeksag · 2018-07-06T08:37:12Z

Modules/_sqlite/connection.c

+                        "should be built with SQLite 3.8.3 or higher");
+        return NULL;
+#else
+        if (sqlite3_libversion_number() < 3008003) {


We usually don't check for runtime library version when doing feature detection, but I think this is better. (Note that the only other check was added only in 2016: 7bea234)

+1 for the runtime check

Checking the runtime version is fine. SQLITE_VERSION_NUMBER is the build version. Using Linux packages, it's common that the machine building packages and the users installing packages use different versions.

It also seems risky to use the flag on old versions according to the discussion in this PR.

bedevere-bot · 2018-07-06T08:37:52Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

sir-sigurd · 2018-07-06T11:26:15Z

I have made the requested changes; please review again

bedevere-bot · 2018-07-06T11:26:18Z

Thanks for making the requested changes!

@berkerpeksag, @vstinner: please review the changes made to this pull request.

vstinner

Almost good! I just have a minor request on the error message.

vstinner · 2018-07-06T15:03:48Z

Lib/sqlite3/test/userfunctions.py

+    @unittest.skipIf(sqlite.sqlite_version_info >= (3, 8, 3), "SQLite < 3.8.3 needed")
+    def CheckFuncDeterministicNotSupported(self):
+        with self.assertRaises(sqlite.NotSupportedError):
+            self.con.create_function("deterministic", 0, int, deterministic=True)


fixed in the following test

vstinner · 2018-07-06T15:04:46Z

Modules/_sqlite/connection.c

+#else
+        if (sqlite3_libversion_number() < 3008003) {
+            PyErr_SetString(pysqlite_NotSupportedError,
+                            "requires SQLite 3.8.3 or higher");


would you mind to add "deterministic"? "deterministic requires SQLite 3.8.3 or higher"

maybe even "deterministic=True requires SQLite 3.8.3 or higher"?

yeah, =True is maybe even better (more explicit).

vstinner · 2018-07-06T15:05:08Z

Modules/_sqlite/connection.c

+    if (deterministic) {
+#if SQLITE_VERSION_NUMBER < 3008003
+        PyErr_SetString(pysqlite_NotSupportedError,
+                        "should be built with SQLite 3.8.3 or higher");


I suggest to reuse the same error message: "deterministic requires SQLite 3.8.3 or higher"

vstinner · 2018-07-06T15:05:32Z

Modules/_sqlite/connection.c

+                            "requires SQLite 3.8.3 or higher");
+            return NULL;
+        }
+        else {


"else {" can be removed, since the if block returns.

vstinner · 2018-07-06T15:08:37Z

Modules/_sqlite/connection.c

+                        "should be built with SQLite 3.8.3 or higher");
+        return NULL;
+#else
+        if (sqlite3_libversion_number() < 3008003) {


+1 for the runtime check

Checking the runtime version is fine. SQLITE_VERSION_NUMBER is the build version. Using Linux packages, it's common that the machine building packages and the users installing packages use different versions.

It also seems risky to use the flag on old versions according to the discussion in this PR.

vstinner

LGTM.

vstinner · 2018-07-06T16:08:35Z

@serhiy-storchaka, @berkerpeksag: The change LGTM. Would you mind to also review the latest version?

berkerpeksag · 2018-07-06T16:53:17Z

Doc/library/sqlite3.rst

+      called as the SQL function. If *deterministic* is true, the created function
+      is marked as `deterministic <https://sqlite.org/deterministic.html>`_, which
+      allows SQLite to perform additional optimizations. This flag is supported by
+      SQLite 3.8.3 or higher, ``sqlite3.NotSupportedError`` will be raised if used


I was going to suggest add a link to the NotSupportedError documentation, then I noticed it wasn't documented. I've opened https://bugs.python.org/issue34061

berkerpeksag

LGTM too. I will wait for a day or two to let Serhiy have a chance to review the latest revision, and merge it.

Thanks!

serhiy-storchaka

LGTM if LGT @berkerpeksag.

I just do not fully understand the test.

serhiy-storchaka · 2018-07-06T17:20:52Z

Lib/sqlite3/test/userfunctions.py

@@ -275,6 +276,24 @@ def CheckAnyArguments(self):
        val = cur.fetchone()[0]
        self.assertEqual(val, 2)

+    @unittest.skipIf(sqlite.sqlite_version_info < (3, 8, 3), "deterministic parameter not supported")
+    def CheckFuncDeterministic(self):
+        mock = unittest.mock.Mock()


Why a mock is used?

It's used to test how much times function is called. If function is non-deterministic, query can't be optimized, so function have to be called 2 times (and actually there is no need to call it at all for this query :-).
This test relies on SQLite's behavior in some degree, but I did't find out a better way to test it.

Thank you for clarification.

Is it worth to add a test for deterministic=False (or for not specified)?

Perhaps we can do something like the following instead of using mock:

def foo(): foo.calls.append(True) foo.calls = []

Then we can check the call count as:

self.assertEqual(len(foo.calls), 1)

@serhiy-storchaka added test for it.
@berkerpeksag not sure why this could be better.

@berkerpeksag This is how I would write this test. But I just don't use mocks much. I think using a mock here is good.

serhiy-storchaka · 2018-07-06T18:09:36Z

Lib/sqlite3/test/userfunctions.py

+    @unittest.skipIf(sqlite.sqlite_version_info >= (3, 8, 3), "SQLite < 3.8.3 needed")
+    def CheckFuncDeterministicNotSupported(self):
+        with self.assertRaises(sqlite.NotSupportedError):
+            self.con.create_function("deterministic", 0, int, deterministic=True)


self.con.create_function("deterministic", 0, int, deterministic=False) should work even on SQLite < 3.8.3, right?

Yes, just pushed the fix.

serhiy-storchaka · 2018-07-06T18:11:35Z

Lib/sqlite3/test/userfunctions.py

@@ -275,6 +276,24 @@ def CheckAnyArguments(self):
        val = cur.fetchone()[0]
        self.assertEqual(val, 2)

+    @unittest.skipIf(sqlite.sqlite_version_info < (3, 8, 3), "deterministic parameter not supported")
+    def CheckFuncDeterministic(self):
+        mock = unittest.mock.Mock()


@berkerpeksag This is how I would write this test. But I just don't use mocks much. I think using a mock here is good.

berkerpeksag · 2018-07-07T16:07:57Z

Lib/sqlite3/test/userfunctions.py

@@ -275,6 +276,28 @@ def CheckAnyArguments(self):
        val = cur.fetchone()[0]
        self.assertEqual(val, 2)

+    def CheckFuncDeterministic(self):


I think this test can be split into three tests:

deterministic=False

deterministic=True if SQLite is older than 3.8.3

deterministic=True if SQLite supports the feature

berkerpeksag · 2018-07-07T16:08:26Z

Lib/sqlite3/test/userfunctions.py

+            self.con.execute(query)
+            self.assertEqual(mock.call_count, 1)
+
+    def CheckFuncDeterministicKwOnly(self):


Nit: Kw -> Keyword

berkerpeksag · 2018-07-08T07:09:30Z

Thanks!

bpo-34041: Added *deterministic* parameter to sqlite3.Connection.crea…

3ea174a

…te_function().

the-knights-who-say-ni added the CLA signed label Jul 4, 2018

bedevere-bot added the awaiting review label Jul 4, 2018

vstinner requested changes Jul 4, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jul 4, 2018

sir-sigurd added 4 commits July 4, 2018 15:52

Made parameter keyword only.

d854d91

Removed redundant line in doc.

5c8a67b

Changed test for SQLite 3.11.

dcb8e51

Do not pass SQLITE_DETERMINISTIC when it's not supported.

25952a3

vstinner reviewed Jul 4, 2018

View reviewed changes

serhiy-storchaka reviewed Jul 4, 2018

View reviewed changes

sir-sigurd added 3 commits July 4, 2018 20:19

Documented parameter as keyword only.

1473689

Added news entry.

faf9b6b

Raise exception on old SQLite versions.

0bfe04a

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 4, 2018

serhiy-storchaka requested a review from berkerpeksag July 6, 2018 06:59

berkerpeksag requested changes Jul 6, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Jul 6, 2018

sir-sigurd added 2 commits July 6, 2018 15:30

Made requested changes.

33f8e64

Fixed indentation.

0dcc22c

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 6, 2018

vstinner reviewed Jul 6, 2018

View reviewed changes

Made requested changes.

dc8ca8d

vstinner approved these changes Jul 6, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jul 6, 2018

berkerpeksag reviewed Jul 6, 2018

View reviewed changes

berkerpeksag approved these changes Jul 6, 2018

View reviewed changes

serhiy-storchaka approved these changes Jul 6, 2018

View reviewed changes

Added test for deterministic=False.

b7d928a

serhiy-storchaka reviewed Jul 6, 2018

View reviewed changes

Run test for deterministic=False even on SQLite < 3.8.3.

3115324

berkerpeksag reviewed Jul 7, 2018

View reviewed changes

Moar changes.

130cb5e

berkerpeksag merged commit 0830858 into python:master Jul 8, 2018

bedevere-bot removed the awaiting merge label Jul 8, 2018

sir-sigurd deleted the sqlite-deterministic-function branch July 8, 2018 07:21

		@@ -0,0 +1,2 @@
		Add the parameter deterministic to the
		sqlite3.Connection.create_function().

bpo-34041: Added *deterministic* parameter to sqlite3.Connection.create_function(). #8086

bpo-34041: Added *deterministic* parameter to sqlite3.Connection.create_function(). #8086

Conversation

sir-sigurd commented Jul 4, 2018 • edited by bedevere-bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sir-sigurd Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sir-sigurd commented Jul 4, 2018

bedevere-bot commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jul 6, 2018

sir-sigurd commented Jul 6, 2018

bedevere-bot commented Jul 6, 2018

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Jul 6, 2018

Choose a reason for hiding this comment

berkerpeksag left a comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkerpeksag commented Jul 8, 2018

bpo-34041: Added deterministic parameter to sqlite3.Connection.create_function(). #8086

bpo-34041: Added deterministic parameter to sqlite3.Connection.create_function(). #8086

sir-sigurd commented Jul 4, 2018 •

edited by bedevere-bot

Loading

sir-sigurd Jul 4, 2018 •

edited

Loading