Opened 9 years ago

Closed 8 years ago

#2519 closed enhancement (fixed)

aligning sqlite3 with postgresql_psycopg2 adapter

Reported by: crankycoder@… Owned by: adrian
Component: Database layer (models, ORM) Version:
Severity: normal Keywords:
Cc: victor.ng@…, adurdin@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

SQLite3 behaves slightly differently than the postgresql_psycopg2 adapter with regards to handling decimal types and using SELECT statements with IN clauses that are empty.

SQLite3 will allow something like :

SELECT * FROM foo WHERE id in ()

That fails horribly in Postgresql.

SQLite3 also returns float back instead of decimal.Decimal for the Django decimal column type. The current driver can't marshall the values into a decimal.Decimal type because the underlying schema definition for SQL decimal types in sqlite is invalid. SQLite can only understand single word type names.

The following is a patch to make the sqlite3 driver use decimal.Decimal type for SQL decimal backed columns as well as raising a SyntaxError exception if a SELECT with an empty IN clause is used.

Using float for a SQL decimal type is not correct since IEEE float values are imprecise and cause problems when dealing with monetary amounts. Cases like dealing with the Euro legally require handling 6 significant digits of precision for all currency exchange - so you really need to use a decimal type.

Index: base.py
===================================================================
--- base.py     (revision 3549)
+++ base.py     (working copy)
@@ -17,7 +17,20 @@
 Database.register_converter("datetime", util.typecast_timestamp)
 Database.register_converter("timestamp", util.typecast_timestamp)
 Database.register_converter("TIMESTAMP", util.typecast_timestamp)
+try:
+    #  this only exists in 2.4+
+    from decimal import Decimal
+    Database.register_converter("numeric", util.typecast_numeric)
+    Database.register_adapter(Decimal, util.rev_typecast_decimal)
+except:
+    pass

+import re
+
+# We need to trap any queries with empty parenthesis because they're invalid in
+# other database - like Postgresql
+empty_in_clause = re.compile(".* +in +\( *\).*", re.I)
+
 def utf8rowFactory(cursor, row):
     def utf8(s):
         if type(s) == unicode:
@@ -74,6 +87,8 @@
     """
     def execute(self, query, params=()):
         query = self.convert_query(query, len(params))
+        if empty_in_clause.match(query):
+            raise SyntaxError, "Invalid SQL query.  Your IN clause is empty. [%s]" % query
         return Database.Cursor.execute(self, query, params)

     def executemany(self, query, param_list):
Index: creation.py
===================================================================
--- creation.py (revision 3549)
+++ creation.py (working copy)
@@ -10,7 +10,7 @@
     'DateTimeField':                'datetime',
     'FileField':                    'varchar(100)',
     'FilePathField':                'varchar(100)',
-    'FloatField':                   'numeric(%(max_digits)s, %(decimal_places)s)',
+    'FloatField':                   'numeric',
     'ImageField':                   'varchar(100)',
     'IntegerField':                 'integer',
     'IPAddressField':               'char(15)',

Attachments (2)

sqlite3.patch (4.2 KB) - added by Victor Ng <victor.ng@…> 9 years ago.
Update to sqlite3 driver to make it more like psycopg2
sqlite3.py (658 bytes) - added by Victor Ng <victor.ng@…> 9 years ago.
unit tests to excercise the delegation patch to SQLiteCursorWrapper

Download all attachments as: .zip

Change History (8)

comment:1 Changed 9 years ago by Victor Ng <crankycoder@…>

Whoops. I forgot to include the diff to django.db.backends.util

This basically - along with the prior patch - will register a convert to PySQLite to convert SQL numeric column types to decimal.Decimal if the current running version of Python allows it. So we don't break compatibility with Python 2.3, but if you *are* running Python 2.4+, you get proper decimal handling.

Index: util.py
===================================================================
--- util.py     (revision 3549)
+++ util.py     (working copy)
@@ -41,7 +41,17 @@
 ###############################################
 # Converters from database (string) to Python #
 ###############################################
+import sys
+version = float('.'.join(map(str, sys.version_info[:2])))
+if version >= 2.4:
+    from decimal import Decimal
+    def typecast_numeric(s):
+        return Decimal(s)
+else:
+    def typecast_numeric(s):
+        return float(s)

+
 def typecast_date(s):
     return s and datetime.date(*map(int, s.split('-'))) or None # returns None if s is null

@@ -89,6 +99,9 @@
 # Converters from Python to database (string) #
 ###############################################

+def rev_typecast_decimal(number):
+    return number.to_eng_string()
+
 def rev_typecast_boolean(obj, d):
     return obj and '1' or '0'


comment:2 Changed 9 years ago by anonymous

  • Cc victor.ng@… added

Changed 9 years ago by Victor Ng <victor.ng@…>

Update to sqlite3 driver to make it more like psycopg2

comment:3 Changed 9 years ago by Victor Ng <victor.ng@…>

  • Component changed from Admin interface to Database wrapper

I've updated the sqlite3 patch.

Changes in this patch:

  • use python 2.4 decimal types if available
  • changes SQLiteCursorWrapper to use delegation to the pysqlite2 driver instead of subclassing as subclassing breaks the fetchmany method of cursor and raw bytestrings come back.

The attached unittest does *not* exercise the decimal coercion as that would break the testsuite under python2.3

Changed 9 years ago by Victor Ng <victor.ng@…>

unit tests to excercise the delegation patch to SQLiteCursorWrapper

comment:4 Changed 8 years ago by Gary Wilson <gary.wilson@…>

  • Has patch set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Design decision needed

I don't think the empty_in_clause is needed any more as of [4283]. It would also be nice to have a single patch (code and tests).

comment:5 Changed 8 years ago by Andrew Durdin <adurdin@…>

  • Cc adurdin@… added

comment:6 Changed 8 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

I don't like the IN() portion of the patch. Feels like it should be fixed at the source (which it is in [4283].

The DecimalField addition in [5203] should fix the other portion.

For the future, please file multiple tickets for multiple changes, so that we can review them in isolation.

Note: See TracTickets for help on using tickets.
Back to Top