sqlencoder upgrade to geoapi filters
There's a long road to internally using GeoAPI filters rather than Geotools filters. This is just a little piece of the work.
I was itching to get the ArcSDE datastore plugin moved completely off of the "old" filters and completely onto the "new" filters.
Most of the ArcSDE datastore plugin moved quite cleanly over, but there were a few bits of main and jdbc that were being pulled in, and which were using deprecated interfaces. So I set about to:
- Clean up SQLEncoder.java, moving it from using "old" filters to "new" filters
- Stop using the SQLUnpacker class and instead use PostPreFilterSplittingVisitor for the SDE internal filter splitting needs
In order to complete this work I had to make two non-trivial decisions that I'd like to run by any interested parties (particularly the main and jdbc module maintainers: jody and cory!) Currently all tests pass and I'm synced to the latest svn revision with no conflicts.
Items need to be completed in descending order.
| Status | Task |
|---|---|
| ✅ | class renaming/package questions answered |
| ✅ | code committed to trunk |
| pending | code QA'd reviewed |
| supported datastores moved from SQLEncoder to 'new' SQLEncoder | |
| unsupported datastores moved from SQLEncoder t 'new' SQLEncoder | |
| ✅ | SDE datastore moved from SQLEncoder to 'new' SQLEncoder |
- Bugging the maintainer to change it
- Changing it myself it it's not maintained (in unsupported)
Trying to clean up SQLEncoder.java turned out to be a bad idea. There are many datastore plugins
which subclass this class as a way of getting easy filter -> SQL encoding support, plus a few
customizations. Changing the underlying methods of SQLEncoder was no good as I don't really have the
expertise to fix all the datastores at the same time.
So instead I created a new port of the class (called 'GeoAPIFilterToSQLEncoder') in the org.geotools.data.jdbc package. This new class is really just a port of the old SQLEncoder class, but made to use "new" filters. I then deprecated the old org.geotools.filter.SQLEncoder class (and it's exceptions) and ported its tests/exceptions over to the new GeoAPIFilterToSQLEncoder.
I left a note in SQLEncoder about which class replaces this now-deprecated class.
QUESTION: What should the name of the new SQLEncoder class be? Is the new different name for the class in a different package a good one? Or should it keep the same name, just change package?
| status | proposal | approach |
|---|---|---|
| ⛔ | Saul's original solution | SQLEncoder gets deprecated. New GeoAPI filter encoder is named GeoAPIFilterToSQLEncoder |
| ⛔ | Saul's alternate suggestion | SQLEncoder gets deprecated. New GeoAPI filter encoder is named identically, but is in different package. Possible confusion due to identical names for different classes. |
| ✅ | Jody's suggestion 1 | SQLEncoder gets deprecated. Long: "FilterToSQL" ArcsdeFilterToSQL, OracleFilterToSQL, PostgisFilterToSQL |
| ⛔ | Jody's suggestion 2 | SQLEncoder gets deprecated. Medium: "FilterEncoder" ArcsdeFilterEncoder, OracleFilterEncoder, PostgisFilterEncoder |
| ⛔ | Jody's suggestion 3 | SQLEncoder gets deprecated. Short: "QL" - ArcsdeSQL, OracleQL, PostGISQL |
| ⛔ | Jody's suggestion 4 | SQLEncoder gets deprecated. Abstract: "Dialect" ArcsdeDialect, OracleDialect, PostGISDialect, |
QUESTION: What should the package be of the new SQLEncoder class (regardless of its name)? The old SQLEncoder lived in org.geotools.filter. SHould the new one live in org.opengis.filter? Or is living in org.geotools.data.jdbc good enough/better?
| status | proposal | approach |
|---|---|---|
| ✅ | Saul's original solution | 'new' SQLEncoder is put into package org.geotools.data.jdbc. Jody's comment on this: org.geotools.data.jdbc is probably for the best' |
| ⛔ | Jody's alternate suggestion | Copy what was done for CQL - 'new' SQLEncoder is put into packgae org.geotools.sql |
After creating and testing this new class, I then ported the SQLEncoderSDE over to use the new class, and everything works great with 100% "new" filters.
Moving from SQLUnpacker to PostPreProcessFilterSplittingVisitor is fairly painless, once you've figured out how PPPFSV works. I ported PPPFSV to use the new filters, and it works just fine now.
FilterCapabilities now represents two things. It represents the capabilites of a filter as expressed with old filters, and the capabilities of a filter as expressed with new filters. This means that the FilterCapabilities class is a bit of a frankenstein, with code handling each individual case/bit in each method and use-case.
QUESTION: What do do with FilterCapabilities?
-
Dual nature is accepted as an "ok thing", and the class is re-architected to account for that.
-
Dual nature is rejected as a confusing/bad thing, and the class is split into an older, deprecated version and a newer version called "FilterCapabilitiesImpl" which implements the org.opengis.filter.capabilities.FilterCapabilitie interface (or whatever the right interface is in geoapi).
This is not the first time this issue has been raised. It's a part of the 2.5.x timeline and it'll raise its head in a few places. Just pointing out some possible routes here. Personally I vote for the latter route.
Jody's comment:
I would like to move towards the opengis.filter.capabilities - but I imagine we will learn a few things (and may need to revise the interfaces - justin already ran into some limitations around Function?).
deprecation:
Index: modules/library/jdbc/src/main/java/org/geotools/filter/SQLEncoder.java
Index: modules/library/jdbc/src/main/java/org/geotools/filter/SQLEncoderException.java
Change to support "new" filters rather than "old" filters
Index: modules/library/main/src/main/java/org/geotools/filter/visitor/PostPreProcessFilterSplittingVisitor.java
Minor changes/cleanup to support the new PostPreProcessFilterSplittingVisitor:
Index: modules/library/jdbc/src/main/java/org/geotools/data/jdbc/DefaultSQLBuilder.java
Index: modules/library/main/src/test/java/org/geotools/filter/visitor/PostPreProcessFilterSplittingVisitorTest.java
Index: modules/library/main/src/test/java/org/geotools/filter/visitor/AbstractPostPreProcessFilterSplittingVisitorTests.java
Index: modules/library/main/src/test/java/org/geotools/filter/visitor/PostPreProcessFilterSplittingVisitorSpatialTest.java
Index: modules/library/main/src/test/java/org/geotools/filter/visitor/PostPreProcessFilterSplitterVisitorFunctionTest.java
Index: modules/plugin/wfs/src/main/java/org/geotools/data/wfs/WFSDataStore.java
More substantial changes were required to get FilterCapabilities to work as a representation of both new and old FilterCapabilities.
Index: modules/library/main/src/main/java/org/geotools/filter/FilterCapabilities.java
New files:
modules/library/jdbc/src/main/java/org/geotools/data/jdbc/GeoAPIFilterToSQLEncoderException.java
modules/library/jdbc/src/main/java/org/geotools/data/jdbc/GeoAPIFilterToSQLEncoder.java
modules/library/jdbc/src/test/java/org/geotools/data/jdbc/SQLFilterSuite.java
modules/library/jdbc/src/test/java/org/geotools/data/jdbc/SQLFilterTestSupport.java
modules/library/jdbc/src/test/java/org/geotools/data/jdbc/GeoAPIFilterToSQLEncoderTest.java