Skip to content

Conversation

@LeOndaz
Copy link
Contributor

@LeOndaz LeOndaz commented Oct 12, 2024

Trac ticket number

ticket-28696

Branch description

Adds support for GeometryType

On human-friendly databases

SELECT GeometryType(geom) AS geom_type FROM spatial_table;

on oracle

SELECT a.geometry.sdo_gtype AS geom_type_code FROM spatial_table a;

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

@LeOndaz
Copy link
Contributor Author

LeOndaz commented Oct 12, 2024

Hello again, I wanted to ask a quick question! What's the recommended way of testing oracle geospatial stuff? I can't find it in the testing pipelines, only others

Also, I'm testing in the pipelines, so I may end up with many commits and squash them later, do we have anything about that? since this surely has to do with the costs

One last thing, are there any hidden tricks to test only a few pipelines and skip the others while testing? specifically oracle

@LeOndaz LeOndaz force-pushed the ticket-28696 branch 3 times, most recently from 37f6669 to e9f562a Compare October 12, 2024 09:26
@LeOndaz LeOndaz marked this pull request as ready for review October 12, 2024 09:27
@felixxm
Copy link
Member

felixxm commented Oct 13, 2024

buildbot, test on oracle.

@LeOndaz
Copy link
Contributor Author

LeOndaz commented Oct 13, 2024

buildbot, test on oracle.

now we're talking, can I also do that at some point?

@felixxm
Copy link
Member

felixxm commented Oct 13, 2024

buildbot, test on oracle.

now we're talking, can I also do that at some point?

Yes you can.

@LeOndaz
Copy link
Contributor Author

LeOndaz commented Oct 21, 2024

buildbot, test on oracle.

@LeOndaz
Copy link
Contributor Author

LeOndaz commented Oct 22, 2024

(I'm commenting to let everyone know that I'm still working on this, but my time is a bit limited)

@LeOndaz
Copy link
Contributor Author

LeOndaz commented Oct 23, 2024

I'm adding context on why I did this approach

I tested all the possible approaches, from .SDO_GTYPE to GET_GTYPE without subquery, Oracle doesn't support embedding the SDO_GEOMETRY.GET_GTYPE directly in CASE or DECODE, so I had to use a subquery to do it

Approaches errors map

  1. {lhs}.SDO_GTYPE => invalid identifier
  2. OGC_GEOMETRYTYPE({lhs}) => wrong number or types of arguments
  3. Using GET_GTYPE as a bounded method {lhs}.GET_GTYPE => invalid identifier
  4. DECODE/CASE with GET_GTYPE => missing expression
  5. I did list all the supported procedures/methods/arguments in my oracle setup and I confirmed the existence of all the methods I did use, I also confirmed their version compatibility and checked DECODE for compatibility with old oracle setups as well
  6. I confirmed that Oracle is by far the most troublesome database that exists between the ones I've tried sqlite, postgres/gis, mysql => brain lag

@LeOndaz
Copy link
Contributor Author

LeOndaz commented Oct 23, 2024

buildbot, test on oracle.

@LeOndaz LeOndaz force-pushed the ticket-28696 branch 2 times, most recently from 8f4aa97 to 94d8671 Compare October 23, 2024 20:12
@LeOndaz
Copy link
Contributor Author

LeOndaz commented Oct 23, 2024

buildbot, test on oracle.

@smithdc1
Copy link
Member

It's OK to not add support Oracle. Someone can always add support later if they are able to find a solution.

If you want to carry on trying to get it to work then that's fine as well!

@LeOndaz
Copy link
Contributor Author

LeOndaz commented Oct 24, 2024

@smithdc1

Thanks for the kind words, It's working on Oracle 23 (as per django-docker-box latest repo linked above by @charettes) and tests are passing fine

But I couldn't find Oracle 19c on their container registry so I can't find it to test locally with it, and the CI error is bugged, it shows a java exception wrapped in python causing giberrish random characters to appear, I will also open a ticket for that bug, if you want to check it out, build on oracle and check the logs

@smithdc1
Copy link
Member

giberrish random characters

Err, do you mean the bit that's in Polish? 🇵🇱

@LeOndaz
Copy link
Contributor Author

LeOndaz commented Oct 24, 2024

@smithdc1 holy shit it's Polish, in short it says Java stopped because of an Exception I missed up so hard, but why is it in Polish though? and any possible clues?

  File "src/oracledb/impl/thin/cursor.pyx", line 196, in oracledb.thin_impl.ThinCursorImpl.execute
  File "src/oracledb/impl/thin/protocol.pyx", line 440, in oracledb.thin_impl.Protocol._process_single_message
  File "src/oracledb/impl/thin/protocol.pyx", line 441, in oracledb.thin_impl.Protocol._process_single_message
  File "src/oracledb/impl/thin/protocol.pyx", line 433, in oracledb.thin_impl.Protocol._process_message
  File "src/oracledb/impl/thin/messages.pyx", line 74, in oracledb.thin_impl.Message._check_and_raise_exception
django.db.utils.DatabaseError: ORA-29532: wywołanie Javy zatrzymane przez nieprzechwycony wyjątek Javy: java.lang.RuntimeException: -2
ORA-06512: przy "MDSYS.SDO_JAVA_STP", linia 76
ORA-06512: przy "MDSYS.SDO_UTIL", linia 6416
ORA-06512: przy "MDSYS.SDO_GEOMETRY", linia 165
ORA-06512: przy linia 1
Help: https://docs.oracle.com/error-help/db/ora-29532/

@felixxm
Copy link
Member

felixxm commented Oct 24, 2024

holy shit it's Polish

Please use a different languages, CoC applies here.

but why is it in Polish though? and any possible clues?

That's how it's configured and we (I) have no time to change it now. It has nothing to do with the fact that it doesn't work.

@LeOndaz
Copy link
Contributor Author

LeOndaz commented Oct 24, 2024

@felixxm I'm not aware of how to change that from my side, I didn't configure anything I just run the buildbot, but hint me and I will change in a PR or smth?

@felixxm
Copy link
Member

felixxm commented Oct 24, 2024

@felixxm I'm not aware of how to change that from my side, I didn't configure anything I just run the buildbot, but hint me and I will change in a PR or smth?

What do you want me to change? There is nothing to change in CI configuration that could help with this patch. Again, "It has nothing to do with the fact that this PR doesn't work".

@charettes
Copy link
Member

@felixxm is there a way to change the locale of the Oracle VM back to English? I'm not familiar with the current setup so I could be missing something. I've also been tripped by the Polish locale in the past and I assume it can be quite confusing for new contributors as well given Oracle error message are often already cryptic in English.

@felixxm
Copy link
Member

felixxm commented Oct 24, 2024

@felixxm is there a way to change the locale of the Oracle VM back to English? I'm not familiar with the current setup so I could be missing something. I've also been tripped by the Polish locale in the past and I assume it can be quite confusing for new contributors as well given Oracle error message are often already cryptic in English.

Yes, we can change this. I will add it to my TODO list 😅

@LeOndaz
Copy link
Contributor Author

LeOndaz commented Jul 17, 2025

buildbot, test on oracle.

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LeOndaz Thanks 👍 I left comments.

I will check later if it works on Oracle 23c, and we can mark it as unsupported on Oracle < 23c.

@felixxm felixxm self-assigned this Jul 17, 2025
@LeOndaz LeOndaz force-pushed the ticket-28696 branch 2 times, most recently from d5ad32c to 176f66f Compare July 17, 2025 06:50
@LeOndaz
Copy link
Contributor Author

LeOndaz commented Jul 17, 2025

buildbot, test on oracle.

@LeOndaz
Copy link
Contributor Author

LeOndaz commented Jul 17, 2025

@felixxm Thank you for your thorough review, I will try my best to make this work with minimal intervention to save time, one quick question to help me in this one and later, how can I get a list of the bot supported commands?

I know it can run oracle from the previous interactions, but just in case, can I add constraints or anything that can help later?

@felixxm
Copy link
Member

felixxm commented Jul 19, 2025

I will check later if it works on Oracle 23c, and we can mark it as unsupported on Oracle < 23c.

It works for me on Oracle 23c 👍 Let's mark it as unsupported on Oracle < 23c. Do you have time to keep working on this? If not I can push final edits.

@felixxm
Copy link
Member

felixxm commented Jul 19, 2025

I pushed some edits, not all but I can continue later.

@felixxm felixxm force-pushed the ticket-28696 branch 3 times, most recently from 5d2d5b3 to ea11ce2 Compare July 19, 2025 13:53
@felixxm felixxm changed the title Fixed #28696 - Added support for GeometryType database function Fixed #33783 -- Added GeometryType GIS database function and __geometry_type lookup. Jul 19, 2025
@felixxm felixxm changed the title Fixed #33783 -- Added GeometryType GIS database function and __geometry_type lookup. Fixed #33783 -- Added GeometryType GIS database function and __geom_type lookup. Jul 19, 2025
@felixxm felixxm changed the title Fixed #33783 -- Added GeometryType GIS database function and __geom_type lookup. Fixed #28696 -- Added GeometryType GIS database function and __geom_type lookup. Jul 19, 2025
@LeOndaz
Copy link
Contributor Author

LeOndaz commented Jul 20, 2025

I will check later if it works on Oracle 23c, and we can mark it as unsupported on Oracle < 23c.

Hey, sorry I was away for a bit, I checked this and it looks good, you did great effort, are we missing anything?

@felixxm
Copy link
Member

felixxm commented Jul 20, 2025

buildbot, test on oracle.

…ype lookup.

Co-Authored-By: Mariusz Felisiak <felisiak.mariusz@gmail.com>
@felixxm
Copy link
Member

felixxm commented Jul 20, 2025

I will check later if it works on Oracle 23c, and we can mark it as unsupported on Oracle < 23c.

Hey, sorry I was away for a bit, I checked this and it looks good, you did great effort, are we missing anything?

No, I think we're ready! Thanks for all updates 👍

@felixxm felixxm merged commit a5b0a61 into django:main Jul 20, 2025
33 of 34 checks passed
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.

4 participants