[Django] #24164: Oracle GIS geoapp extent test failure
Django
-------------------------------------+---------------------------
Reporter: timgraham | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 1.8alpha1
Severity: Normal | Keywords: oracle
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+---------------------------
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'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24164>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: timgraham | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 1.8alpha1
Keywords: oracle | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by shaib):
The failing SQL is:
{{{#!sql
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
{{{#!sql
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:1>
Django
Reporter: timgraham | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 1.8alpha1
Keywords: oracle | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:2>
Django
Reporter: timgraham | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 1.8alpha1
Keywords: oracle | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jarshwah):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:3>
Django
Reporter: timgraham | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 1.8alpha1
Keywords: oracle | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:4>
Django
Reporter: timgraham | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 1.8alpha1
Keywords: oracle | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:5>
Django
Reporter: timgraham | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 1.8alpha1
Keywords: oracle | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:6>
Django
Reporter: timgraham | Owner: timgraham
Type: Bug | Status: assigned
Component: GIS | Version: 1.8alpha1
Severity: Normal | Resolution:
Keywords: oracle | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => assigned
* owner: nobody => timgraham
Comment:
I'll try to fix this with that advice.
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:7>
Django
Reporter: timgraham | Owner: timgraham
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: oracle | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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:
{{{#!sql
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'
}}}
Is the first line of the correct query supposed to be `SELECT
SDO_AGGR_MBR(SDO_AGGR_MBR("__COL1"))` instead?
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:8>
Django
Reporter: timgraham | Owner: timgraham
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: oracle | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/4012 PR] (we'll see if anything
else broke)
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:9>
Django
Reporter: timgraham | Owner: timgraham
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: oracle | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:10>
Django
Reporter: timgraham | Owner: timgraham
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: oracle | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jarshwah):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:11>
Django
Reporter: timgraham | Owner: timgraham
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: oracle | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Ok, I added a mention in the release notes.
The addition of `select_format=True` in the patch fixed the issue in
comment 8.
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:12>
Django
Reporter: timgraham | Owner: timgraham
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: oracle | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
OK, just tested it with my patch and \o/, this works, albeit with this
complementary change:
{{{
#!diff
diff --git a/django/db/models/sql/compiler.py
b/django/db/models/sql/compiler.py
index b92855f..5f41a12 100644
--- a/django/db/models/sql/compiler.py
+++ b/django/db/models/sql/compiler.py
@@ -464,7 +464,7 @@ class SQLCompiler(object):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:13>
Django
Reporter: timgraham | Owner: timgraham
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: oracle | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:14>
Django
Reporter: timgraham | Owner: timgraham
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: oracle | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* stage: Accepted => Ready for checkin
Comment:
(pending naming remarks on PR)
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:15>
Django
Reporter: timgraham | Owner: timgraham
Component: GIS | Version: 1.8alpha1
Severity: Normal | Resolution: fixed
Keywords: oracle | Triage Stage: Ready for checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"29c0073335c7f7cdc482866e093e5e42a42625e5"]:
{{{
#!CommitTicketReference repository=""
revision="29c0073335c7f7cdc482866e093e5e42a42625e5"
Fixed #24164 -- Fixed Oracle GIS limited aggregation test failure.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:16>
Django
Reporter: timgraham | Owner: timgraham
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: oracle | Triage Stage: Ready for checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"df68751134531462f005a75e7342d46540033b88"]:
{{{
#!CommitTicketReference repository=""
revision="df68751134531462f005a75e7342d46540033b88"
[1.8.x] Fixed #24164 -- Fixed Oracle GIS limited aggregation test failure.
Backport of 29c0073335c7f7cdc482866e093e5e42a42625e5 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24164#comment:17>