[rql2sql] closes #1832859: fake HAVING terms w/ EXISTS terms
authorSylvain Thénault <sylvain.thenault@logilab.fr>
Wed, 20 Jul 2011 14:10:03 +0200
changeset 7672 f31f9882c90f
parent 7671 d911a73ac8c5
child 7673 f9227b9d6183
[rql2sql] closes #1832859: fake HAVING terms w/ EXISTS terms there are cases where we want to use the HAVING trick on terms that should belong to an EXISTS subquery. Currently , this causes bad sql. We should fix that until we've a proper rql grammar avoiding the need for this trick. Notice at some point, we may want **actuall** HAVING clauses and GROUPBY used in EXISTS subquery, but that's another story.
server/sources/rql2sql.py
server/test/unittest_rql2sql.py
--- a/server/sources/rql2sql.py	Wed Jul 20 14:10:02 2011 +0200
+++ b/server/sources/rql2sql.py	Wed Jul 20 14:10:03 2011 +0200
@@ -56,6 +56,7 @@
 from logilab.database import FunctionDescr, SQL_FUNCTIONS_REGISTRY
 
 from rql import BadRQLQuery, CoercionError
+from rql.utils import common_parent
 from rql.stmts import Union, Select
 from rql.nodes import (SortTerm, VariableRef, Constant, Function, Variable, Or,
                        Not, Comparison, ColumnAlias, Relation, SubQuery, Exists)
@@ -669,7 +670,7 @@
             else:
                 tocheck.append(compnode)
         # tocheck hold a set of comparison not implying an aggregat function
-        # put them in fakehaving if the don't share an Or node as ancestor
+        # put them in fakehaving if they don't share an Or node as ancestor
         # with another comparison containing an aggregat function
         for compnode in tocheck:
             parents = set()
@@ -784,7 +785,20 @@
         sorts = select.orderby
         groups = select.groupby
         having = select.having
-        morerestr = extract_fake_having_terms(having)
+        for restr in extract_fake_having_terms(having):
+            scope = None
+            for vref in restr.get_nodes(VariableRef):
+                vscope = vref.variable.scope
+                if vscope is select:
+                    continue # ignore select scope, so restriction is added to
+                             # the inner most scope possible
+                if scope is None:
+                    scope = vscope
+                elif vscope is not scope:
+                    scope = common_parent(scope, vscope).scope
+            if scope is None:
+                scope = select
+            scope.add_restriction(restr)
         # remember selection, it may be changed and have to be restored
         origselection = select.selection[:]
         # check if the query will have union subquery, if it need sort term
@@ -829,7 +843,7 @@
         self._in_wrapping_query = False
         self._state = state
         try:
-            sql = self._solutions_sql(select, morerestr, sols, distinct,
+            sql = self._solutions_sql(select, sols, distinct,
                                       needalias or needwrap)
             # generate groups / having before wrapping query selection to get
             # correct column aliases
@@ -900,15 +914,13 @@
                 except KeyError:
                     continue
 
-    def _solutions_sql(self, select, morerestr, solutions, distinct, needalias):
+    def _solutions_sql(self, select, solutions, distinct, needalias):
         sqls = []
         for solution in solutions:
             self._state.reset(solution)
             # visit restriction subtree
             if select.where is not None:
                 self._state.add_restriction(select.where.accept(self))
-            for restriction in morerestr:
-                self._state.add_restriction(restriction.accept(self))
             sql = [self._selection_sql(select.selection, distinct, needalias)]
             if self._state.restrictions:
                 sql.append('WHERE %s' % ' AND '.join(self._state.restrictions))
--- a/server/test/unittest_rql2sql.py	Wed Jul 20 14:10:02 2011 +0200
+++ b/server/test/unittest_rql2sql.py	Wed Jul 20 14:10:03 2011 +0200
@@ -1643,12 +1643,26 @@
                     '''SELECT (A || _X.cw_ref)
 FROM cw_Affaire AS _X''')
 
-    def test_or_having_fake_terms(self):
+    def test_or_having_fake_terms_base(self):
         self._check('Any X WHERE X is CWUser, X creation_date D HAVING YEAR(D) = "2010" OR D = NULL',
                     '''SELECT _X.cw_eid
 FROM cw_CWUser AS _X
 WHERE ((CAST(EXTRACT(YEAR from _X.cw_creation_date) AS INTEGER)=2010) OR (_X.cw_creation_date IS NULL))''')
 
+    def test_or_having_fake_terms_exists(self):
+        # crash with rql <= 0.29.0
+        self._check('Any X WHERE X is CWUser, EXISTS(B bookmarked_by X, B creation_date D) HAVING D=2010 OR D=NULL, D=1 OR D=NULL',
+                    '''SELECT _X.cw_eid
+FROM cw_CWUser AS _X
+WHERE EXISTS(SELECT 1 FROM bookmarked_by_relation AS rel_bookmarked_by0, cw_Bookmark AS _B WHERE rel_bookmarked_by0.eid_from=_B.cw_eid AND rel_bookmarked_by0.eid_to=_X.cw_eid AND ((_B.cw_creation_date=1) OR (_B.cw_creation_date IS NULL)) AND ((_B.cw_creation_date=2010) OR (_B.cw_creation_date IS NULL)))''')
+
+    def test_or_having_fake_terms_nocrash(self):
+        # crash with rql <= 0.29.0
+        self._check('Any X WHERE X is CWUser, X creation_date D HAVING D=2010 OR D=NULL, D=1 OR D=NULL',
+                    '''SELECT _X.cw_eid
+FROM cw_CWUser AS _X
+WHERE ((_X.cw_creation_date=1) OR (_X.cw_creation_date IS NULL)) AND ((_X.cw_creation_date=2010) OR (_X.cw_creation_date IS NULL))''')
+
     def test_not_no_where(self):
         # XXX will check if some in_group relation exists, that's it.
         # We  can't actually know if we want to check if there are some
@@ -1699,7 +1713,7 @@
     def test_regexp(self):
         self.skipTest('regexp-based pattern matching not implemented in sqlserver')
 
-    def test_or_having_fake_terms(self):
+    def test_or_having_fake_terms_base(self):
         self._check('Any X WHERE X is CWUser, X creation_date D HAVING YEAR(D) = "2010" OR D = NULL',
                     '''SELECT _X.cw_eid
 FROM cw_CWUser AS _X
@@ -1984,7 +1998,7 @@
             yield t
 
 
-    def test_or_having_fake_terms(self):
+    def test_or_having_fake_terms_base(self):
         self._check('Any X WHERE X is CWUser, X creation_date D HAVING YEAR(D) = "2010" OR D = NULL',
                     '''SELECT _X.cw_eid
 FROM cw_CWUser AS _X
@@ -2095,7 +2109,7 @@
 FROM cw_Personne AS _P''')
 
 
-    def test_or_having_fake_terms(self):
+    def test_or_having_fake_terms_base(self):
         self._check('Any X WHERE X is CWUser, X creation_date D HAVING YEAR(D) = "2010" OR D = NULL',
                     '''SELECT _X.cw_eid
 FROM cw_CWUser AS _X