Andrea and all, I've attached a patch to http://jira.codehaus.org/browse/GEOT-2182 for your review, or if anyone else is interested in reviewing will be more than welcome.
The comment on the issue already has a summary of what was accomplished, and it survives a full build with -Dall and postgis online tests enabled. Looking forward to your comments. Gabriel On Friday 05 December 2008 16:05:07 Gabriel Roldán wrote: > Hi Andrea, thanks a lot for the insights. > please see comments below. > > On Thursday 04 December 2008 17:03:04 Andrea Aime wrote: > > Gabriel Roldán ha scritto: > > > Hi Andrea, > > > > > > giving a run at simplifying filters for jdbc datastores (for instance, > > > postgis) I have some doubts on where it'd be best to do so, since I'm > > > not an expert on the jdbc datastores I thought I may use your thoughts > > > once more to not mess it up :) > > > > > > So, we have this nice issue (http://jira.codehaus.org/browse/GEOT-2182) > > > about wiping out invalid fids that aims to fix what seems to be a > > > pretty common mistake on WFS's, send a fid filter over typeName1 with a > > > fid like "typeName2.someting" and you'll still get a non empty result > > > (I have seen the defect on a commercial one). > > > > > > But for jdbc ones I guess we should add a isValid(String fid):boolean > > > method to FIDMapper and then feed the SimplifyingFilterVisitor with an > > > adaptor FIDMapper->FIDValidator. > > > This way related things keep close instead of requiring a big switch > > > statement in order to decide which validation strategy is appropriate > > > for a given fidmapper. > > > > > > Now, about where to apply this, for postgis I guess it would be at > > > DefaultSQLBuilder.splitFilter(Filter filter), but I couldn't find out > > > where to get the FIDMapper in use at this point. The closer I get to is > > > that SQLBuilder contains a SQLEncoder which in turn has a > > > setFidMapper(FIDMapper) mehod, but no getter. So... I'm open to any > > > suggestion on how we should better hanlde this for postgis and jdbc in > > > general. please? > > > > Well, if you look at the current code you'll see that the simplifying > > filter visitor has been used only in JDBCFeatureStore.getFeatureReader. > > There you can get the fid mapper, but as you have noticed, it's not > > a general enough position, as it leaves out FeatureSource methods > > such as bulk updates/deletes, and in general all writes. > > that's right... > > > Given that SqlEncoder is fid mapper away anyways your proposal to > > expose the fid mapper in a getter sounds good, and solves the > > above by allowing other places to benefit from the filter > > simplification. > > > > The best place to inject the fid mapper seems to be > > JDBC1DataStore.getSqlBuilder(), but pay attention, > > that method is overridden in most JDBC data stores. > > Actually what I'm playing with is to perform the fid simplification at > SQLBuilder.getPre/PostQueryFilter(Filter) since that one seems to be called > both for queries and transactions (ie, getFeatureReader, getFeatureWriter, > modifyFeatures and removeFeatures). > Do you think I am missing something here? it seems to me like a pretty good > place where to perform the simplification: right at the pre/post filter > splitting stage. > > > Given the magnitude of this change I would be more comfortable > > about: > > - having someone review it > > I was taking for granted I can abuse of your time here :) > > > - commit it on 2.5.x only after GeoServer 1.7.1 is released > > sure thing! > > Thanks, > > Gabriel > > > Cheers > > Andrea ------------------------------------------------------------------------------ SF.Net email is Sponsored by MIX09, March 18-20, 2009 in Las Vegas, Nevada. The future of the web can't happen without you. Join us at MIX09 to help pave the way to the Next Web now. Learn more and register at http://ad.doubleclick.net/clk;208669438;13503038;i?http://2009.visitmix.com/ _______________________________________________ Geotools-devel mailing list Geotools-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geotools-devel
