Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #29955 -- Added support for distance expression to the dwithin lookup. #11681

Merged
merged 1 commit into from
Aug 24, 2019

Conversation

charettes
Copy link
Member

@charettes charettes commented Aug 17, 2019

This was missed when adding support to other distance lookups in refs #25499 (37d06cf)

Thanks Peter Bex for the report and Mariusz for the testcase.

@charettes charettes force-pushed the ticket-29955 branch 2 times, most recently from 93433d8 to f09dbfc Compare August 17, 2019 14:42
@charettes
Copy link
Member Author

Also included changes similar to 00db71d for Oracle support.

@charettes
Copy link
Member Author

charettes commented Aug 17, 2019

From a little investigation it looks like Oracle's SDO_WITHIN_DISTANCE operator doesn't support expressions and that the less performant SDO_GEOM.WITHIN_DISTANCE function must be used in these cases.

The thing is spatial operators don't have access to rhs to determine whether or not it's an expression (they simply get passed compiled SQL) so we'd need to override DWithinLookup.as_oracle to perform some different stuff if hasattr(dist_param, 'resolve_expression').

This is theoretical since I didn't manage to get an Oracle-GIS setup working locally and I'm frankly unsure of what SDO_GEOM.WITHIN_DISTANCE is expecting for dim1 and dim2 so I'd suggest we add a supports_dwithin_distance_expr = False feature flag for Oracle and move on for now.

@felixxm
Copy link
Member

felixxm commented Aug 17, 2019

@jtiai Can you take a look on an Oracle-GIS issue?

@claudep
Copy link
Member

claudep commented Aug 18, 2019

I would suggest splitting the issue in two, and committing the non-Oracle part ASAP.

@felixxm
Copy link
Member

felixxm commented Aug 18, 2019

Agreed, let's add a database feature for now. I tried different formats, e.g.

  • ..., 'distance = '||%(value)s)
  • ..., 'distance = %(value)s)'

but without any results.

@felixxm felixxm self-assigned this Aug 18, 2019
@felixxm felixxm force-pushed the ticket-29955 branch 2 times, most recently from 59cc32b to 9b67e4b Compare August 23, 2019 12:24
@felixxm
Copy link
Member

felixxm commented Aug 23, 2019

@charettes I added feature flag and an extra test for Oracle.

@felixxm felixxm requested a review from claudep August 23, 2019 12:26
@claudep
Copy link
Member

claudep commented Aug 23, 2019

Looks good (failure is due to a recent commit of mine, unrelated to this patch). Thanks!

@felixxm
Copy link
Member

felixxm commented Aug 23, 2019

@claudep Thanks! I sent separate PR to fix falling tests #11706 on Oracle.

…lookup.

This was missed when adding support to other distance lookups in
refs #25499.

Thanks Peter Bex for the report and Mariusz for testcases.
@felixxm felixxm removed the request for review from claudep August 23, 2019 19:30
@felixxm felixxm merged commit bb9e82f into django:master Aug 24, 2019
@charettes charettes deleted the ticket-29955 branch August 30, 2019 22:32
@charettes
Copy link
Member Author

Thanks for shepherding the commit to master @felixxm 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants