Oracle GIS geoapp extent test failure

[Tim Graham]

The new test added in 374c2419e5adef53a643bf69c4753a6bf0c78a98 doesn't work on Oracle. We need to fix Oracle or skip the test.

======================================================================
ERROR: test_extent_with_limit (django.contrib.gis.tests.geoapp.tests.GeoQuerySetTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tim/code/django/django/test/testcases.py", line 1005, in skip_wrapper
    return test_func(*args, **kwargs)
  File "/home/tim/code/django/django/contrib/gis/tests/geoapp/tests.py", line 502, in test_extent_with_limit
    extent2 = City.objects.all()[:3].aggregate(Extent('point'))['point__extent']
  File "/home/tim/code/django/django/db/models/query.py", line 300, in aggregate
    return query.get_aggregation(self.db, kwargs.keys())
  File "/home/tim/code/django/django/db/models/sql/query.py", line 428, in get_aggregation
    result = compiler.execute_sql(SINGLE)
  File "/home/tim/code/django/django/db/models/sql/compiler.py", line 826, in execute_sql
    cursor.execute(sql, params)
  File "/home/tim/code/django/django/db/backends/utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
  File "/home/tim/code/django/django/db/utils.py", line 95, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/home/tim/code/django/django/db/backends/utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
  File "/home/tim/code/django/django/db/backends/oracle/base.py", line 478, in execute
    return self.cursor.execute(query, self._param_generator(params))
DatabaseError: ORA-06553: PLS-306: wrong number or types of arguments in call to 'SDO_AGGR_MBR'

[Shai Berger]

The failing SQL is:

SELECT SDO_AGGR_MBR("__COL1") FROM (
   SELECT * FROM (
      SELECT "_SUB".*, ROWNUM AS "_RN" FROM (
         SELECT "GEOAPP_CITY"."ID" AS Col1, "GEOAPP_CITY"."NAME" AS Col2,
                SDO_UTIL.TO_WKTGEOMETRY("GEOAPP_CITY"."POINT") AS Col3, 
                SDO_UTIL.TO_WKTGEOMETRY("GEOAPP_CITY"."POINT") AS "__COL1" 
         FROM "GEOAPP_CITY"
      ) "_SUB" WHERE ROWNUM <= 3
   ) WHERE "_RN" > 0) subquery

An earlier query which succeeds in that test is

SELECT SDO_UTIL.TO_WKTGEOMETRY(SDO_AGGR_MBR("GEOAPP_CITY"."POINT")) AS "POINT__EXTENT"
FROM "GEOAPP_CITY"

So it seems the problem is that SDO_UTIL.TO_WKTGEOMETRY is being called "too soon"; it apparently needs to be called on geometric result values in order to return the right type to the user, but here it is applied to an intermediate result (before it is passed to the aggregate).

In particular, this means there is a real problem here, not just a testing problem; skipping is the wrong solution.

[Claude Paroz]

I've encountered similar issues on other tests with SQLite and MySQL AsText converters. Ideally, the subquery should know it is a subquery and not call select_format when compiling.

[Josh Smeaton]

Yes this does look like the same problem. Claude's idea sounds like the way to go. Did you open a ticket for the AsText issues? And what is the failing AsText test? It'll be easier to test against those backends rather than oragis, and the solution should translate to both.

[Claude Paroz]

I encountered this issue while working on #14483. This is the commit that passes with PostGIS but fails with MySQL or Spatialite (and most probably Oracle too) because of the select artifact:
​https://github.com/claudep/django/commit/53783c93a551fe769d5395e6ee54d01729b23722

[Tim Graham]

If no one fixes this issue in the next couple days I plan to mark the test as @expectedFailure on Oracle so we get the build back to green and don't miss any new failures that are introduced.

[Anssi Kääriäinen]

The select_format is pretty much equivalent to from_db_value, but done on the database side (because we don't know how to do it on Python side). So, it must be done only at the point where the expression is in the outermost select list.

Adding an is_subquery flag to compiler, setting this to true for the inner query, and finally skipping the select_format generation for subqueries seems like a good enough fix to me.

[Tim Graham]

I'll try to fix this with that advice.

[Tim Graham]

WIP: ​https://github.com/timgraham/django/compare/django:master...timgraham:24164?expand=1

So far I got rid of the inner SDO_UTIL.TO_WKTGEOMETRY calls so the query is now:

SELECT SDO_AGGR_MBR("__COL1") FROM (
    SELECT * FROM (
        SELECT "_SUB".*, ROWNUM AS "_RN" FROM (
            SELECT "GEOAPP_CITY"."ID", "GEOAPP_CITY"."NAME",
                "GEOAPP_CITY"."POINT", "GEOAPP_CITY"."POINT" AS "__COL1" 
            FROM "GEOAPP_CITY"
        ) "_SUB" WHERE ROWNUM <= 3
    ) WHERE "_RN" > 0
) subquery

but there's a Python error:

======================================================================
ERROR: test_extent_with_limit (django.contrib.gis.tests.geoapp.tests.GeoQuerySetTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tim/code/django/django/test/testcases.py", line 1005, in skip_wrapper
    return test_func(*args, **kwargs)
  File "/home/tim/code/django/django/contrib/gis/tests/geoapp/tests.py", line 519, in test_extent_with_limit
    extent2 = City.objects.all()[:3].aggregate(Extent('point'))['point__extent']
  File "/home/tim/code/django/django/db/models/query.py", line 306, in aggregate
    return query.get_aggregation(self.db, kwargs.keys())
  File "/home/tim/code/django/django/db/models/sql/query.py", line 428, in get_aggregation
    result = compiler.apply_converters(result, converters)
  File "/home/tim/code/django/django/db/models/sql/compiler.py", line 777, in apply_converters
    value = converter(value, self.connection, self.query.context)
  File "/home/tim/code/django/django/contrib/gis/db/models/aggregates.py", line 48, in convert_value
    return connection.ops.convert_extent(value, context.get('transformed_srid'))
  File "/home/tim/code/django/django/contrib/gis/db/backends/oracle/operations.py", line 142, in convert_extent
    ext_geom = Geometry(clob.read(), srid)
AttributeError: 'cx_Oracle.OBJECT' object has no attribute 'read'

It seems the first line of the query should be SELECT SDO_UTIL.TO_WKTGEOMETRY(SDO_AGGR_MBR("__COL1")).

[Tim Graham]

​PR (we'll see if anything else broke)

[Tim Graham]

Build passes (also checked Oracle and Oracle GIS)! Assuming the code looks fine, should the addition of the subquery parameter to as_sql() be documented as a private API change?

[Josh Smeaton]

Yes, I think it should be documented in some manner as this is the most public of private backend apis.

Did you solve the way the SQL was generated from comment:8? I guess so since all tests pass, but was there something particularly tricky that'd be helpful to know for next time?

Claude, does your patch (​https://github.com/claudep/django/commit/53783c93a551fe769d5395e6ee54d01729b23722) work properly with this one here? It seems to solve the inner AsText problem you were seeing.

LGTM though. I'm sure we'll find other uses for the subquery flag now that it exists.

[Tim Graham]

Ok, I added a mention in the release notes.

The addition of select_format=True in the patch fixed the issue in comment 8.

[Claude Paroz]

OK, just tested it with my patch and \o/, this works, albeit with this complementary change:

django/db/models/sql/compiler.py

@@ -464 +464 @@
         if obj.low_mark == 0 and obj.high_mark is None and not self.query.distinct_fields:
             # If there is no slicing in use, then we can safely drop all ordering
             obj.clear_ordering(True)
-        return obj.get_compiler(connection=self.connection).as_sql()
+        return obj.get_compiler(connection=self.connection).as_sql(subquery=True)
 
     def get_default_columns(self, start_alias=None, opts=None, from_parent=None):
         """

I guess we can assume that subquery is always True when as_sql is called from as_nested_sql. I can add this change in my patch, or it can be added in the current one, as you wish, Tim.

[Tim Graham]

I think it's fine to include it with yours for clarity of why it's needed. Both patches seem appropriate to backport to 1.8.

[Claude Paroz]

(pending naming remarks on PR)

[Tim Graham <timograham@…>]

In 29c0073335c7f7cdc482866e093e5e42a42625e5:

Fixed #24164 -- Fixed Oracle GIS limited aggregation test failure.

[Tim Graham <timograham@…>]

In df68751134531462f005a75e7342d46540033b88:

[1.8.x] Fixed #24164 -- Fixed Oracle GIS limited aggregation test failure.

Backport of 29c0073335c7f7cdc482866e093e5e42a42625e5 from master