Farber, Saul (ENV) wrote: > Hello all, > > I've chatted with a few of you over the last week or two about moving some of > the internals of geotools from "old" (org.geotools.filter) filter usage to > "new" (org.opengis.filter) filter usage. > Yeah. > 1) 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. > Makes sense. > 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. > Nice approach. > 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? > Wonder if we can play with the subclass naming convention: The idea is we go from "filter" to "sql" (but the sql dialect specific for the target database). - Long: "FilterToSQL" - ArcsdeFilterToSQL, OracleFilterToSQL, PostgisFilterToSQL - Midium: "FilterEncoder" - ArcsdeFilterEncoder, OracleFilterEncoder, PostgisFilterEncoder - Short: "QL" - ArcsdeSQL, OracleQL, PostGISQL - 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? > We will stick with "org.geotools" - some ideas: - Copy what was done for CQL - org.geotools.sql org.geotools.data.jdbc is probably for the best. > WRAPUP: 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. > Woot! Way to go Sual. > One comment: > 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. I would suggest that either: > a. This is accepted as an ok thing, and the class is re-architected to > account for that. > b. This is rejected as a *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.FilterCapabilities interface > (or whatever the right interface is in geoapi). > 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?). > Ok, here's a list of files changed and the nature of the changes: > > 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 > You may indeed want to separate that out ... can a change of interface help you? Can we make both old and new available as separate interfaces? > 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 > Cool. > Big question: > > Jody/cory/whoever. Should I commit this? Does this qualify as a > GTIP/big-change that needs voting or more formal documentation? Do you want > to see a copy of it to review it? > If we need to drag the geoapi FilterCapabilities into the mix (it would of been a mistake if this was not already done) then it would be polite to do the proposal thing (make sure we capture the BEFORE / AFTER code examples prior to doing the work). I am more concerned about impacting client code (rather then JDBCDataStore subclasses); although it sounds like you have little exposure to them? Let's make a proposal - just so we have a checklist to follow (merge, review, update docs etc...) > I'm committed to tracking down any code that uses the old SQLUnpacker or > SQLEncoder classes and either: > a. Bugging the maintainer to change it > b. Changing it myself it it's not maintained (in unsupported) > > Not sure if that changes the answer to the above question. > Just means your name is put on more of the check list :-) > Sorry for the long one! > Cheers, and thanks for all the hard work. Jody ------------------------------------------------------------------------- Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV _______________________________________________ Geotools-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geotools-devel
