Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#24164 closed Bug (fixed)

Oracle GIS geoapp extent test failure

Reported by: Tim Graham Owned by: Tim Graham
Component: GIS Version: 1.8alpha1
Severity: Normal Keywords: oracle
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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'

Change History (17)

comment:1 Changed 5 years ago by 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.

comment:2 Changed 5 years ago by 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.

comment:3 Changed 5 years ago by 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.

comment:4 Changed 5 years ago by 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

comment:5 Changed 5 years ago by 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.

comment:6 Changed 5 years ago by 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.

comment:7 Changed 5 years ago by Tim Graham

Owner: changed from nobody to Tim Graham
Status: newassigned

I'll try to fix this with that advice.

comment:8 Changed 5 years ago by 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")).

Last edited 5 years ago by Tim Graham (previous) (diff)

comment:9 Changed 5 years ago by Tim Graham

Has patch: set

PR (we'll see if anything else broke)

comment:10 Changed 5 years ago by 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?

comment:11 Changed 5 years ago by 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.

comment:12 Changed 5 years ago by 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.

comment:13 Changed 5 years ago by Claude Paroz

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

  • django/db/models/sql/compiler.py

    diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py
    index b92855f..5f41a12 100644
    a b class SQLCompiler(object): 
    464464        if obj.low_mark == 0 and obj.high_mark is None and not self.query.distinct_fields:
    465465            # If there is no slicing in use, then we can safely drop all ordering
    466466            obj.clear_ordering(True)
    467         return obj.get_compiler(connection=self.connection).as_sql()
     467        return obj.get_compiler(connection=self.connection).as_sql(subquery=True)
    468468
    469469    def get_default_columns(self, start_alias=None, opts=None, from_parent=None):
    470470        """

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.

comment:14 Changed 5 years ago by 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.

comment:15 Changed 5 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

(pending naming remarks on PR)

comment:16 Changed 5 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 29c0073335c7f7cdc482866e093e5e42a42625e5:

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

comment:17 Changed 5 years ago by Tim Graham <timograham@…>

In df68751134531462f005a75e7342d46540033b88:

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

Backport of 29c0073335c7f7cdc482866e093e5e42a42625e5 from master

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