Wow you certainly find the epic bugs.
Reading your patch you you use converter to "cast" the objects to the same
type:
Object other = expLit.evaluate(null, this.literal.getClass());
return other != null && other.equals(this.literal);
I would feel more comfortable adding an extra fallback if this cast does
not work. Casting both to String (since that is how Filter being an XML
spec plays the game):
Object other = expLit.evaluate(null, this.literal.getClass());
if( other != null ){
return other.equals(this.literal);
else {
String str1 = this.evaulate( null, String.class );
String str2 = expLit.evaulate( null, String.class );
return str1 != null && str2 != null && str1.equals(str2);
}
I would like to proceed with fixing LiteralExpressionImpl as it is indeed
very broken like that. I will be sure to highlight it in release notes :)
Jody Garnett
On Wed, Feb 12, 2014 at 7:37 AM, Andrea Aime
<[email protected]>wrote:
> Hi,
> my recent improvements to the SimplifyingFilterVisitor unearthed a really
> scary
> problem in LiteralExpressionImpl equals implementation, which in turn
> resulting in
> more bugs being found.
>
> The changes are not that many, but I could use some review/feedback.
>
> So... the SimplifyingFilterVisitor now basically removes duplicate filters
> from an and-ed
> or or-ed list of filters. Nothing fancy no, "a = 1 and a = 1" --> a = 1,
> "a = 1 or a = 1" --> a = 1.
> Except that due the way LiteralExpressionImpl equality is implemented
> today,
> "a = 2012-10-2 and a = 2010-6-27" --> a = 2012-10-2 !!!!
> What? Yes, have a look at LiteralExpressionImpl equals here:
>
> https://github.com/geotools/geotools/blob/master/modules/library/main/src/main/java/org/geotools/filter/LiteralExpressionImpl.java#L250
> If the object type being compared is not one of the base one (Date being a
> case), we just say the two literals
> are equal, without any further test, even if they are blatantly different!
>
> So ok, simple change here:
>
> https://github.com/geotools/geotools/pull/361/files#diff-5b400a8367c44432b2ad0dad97cd5b0cL274
>
> This in turn showed that some of the ECQL parser building methods were
> wrongly implemented,
> and some of the tests were not taking into account the timezone, resulting
> in more changes.
> Finally some filters were built using Instant instead of Date, so had to
> roll out a converter extension
> to make the two comparable.
>
> Full changeset here:
> https://github.com/geotools/geotools/pull/361
>
> Now, I've made a full GeoTools 10.x build, all fine, and a full Geoserver
> 2.4.x build, which fixes the build breakage
> we have been seeing for some days now in wms.
> I actually still have failures in the importer module, the usual mosaic
> with time, but I've found I have those regardless
> of the changes in the above pull request (I have rebuilt everything
> against vanilla 10.x, same errors)....
>
> Hmmm... for the time being maybe I'll roll back the
> SimplifyingFilterVisitor optimizations from 10.x
> and only commit on trunk? Yet, LiteralExpressionImpl seems just horribly
> broken like that...
>
> Opinions?
>
> Cheers
> Andrea
>
> --
> == Our support, Your Success! Visit http://opensdi.geo-solutions.it for
> more information ==
>
> Ing. Andrea Aime
> @geowolf
> Technical Lead
>
> GeoSolutions S.A.S.
> Via Poggio alle Viti 1187
> 55054 Massarosa (LU)
> Italy
> phone: +39 0584 962313
> fax: +39 0584 1660272
> mob: +39 339 8844549
>
> http://www.geo-solutions.it
> http://twitter.com/geosolutions_it
>
> -------------------------------------------------------
>
>
> ------------------------------------------------------------------------------
> Android apps run on BlackBerry 10
> Introducing the new BlackBerry 10.2.1 Runtime for Android apps.
> Now with support for Jelly Bean, Bluetooth, Mapview and more.
> Get your Android app in front of a whole new audience. Start now.
>
> http://pubads.g.doubleclick.net/gampad/clk?id=124407151&iu=/4140/ostg.clktrk
> _______________________________________________
> GeoTools-Devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/geotools-devel
>
>
------------------------------------------------------------------------------
Android apps run on BlackBerry 10
Introducing the new BlackBerry 10.2.1 Runtime for Android apps.
Now with support for Jelly Bean, Bluetooth, Mapview and more.
Get your Android app in front of a whole new audience. Start now.
http://pubads.g.doubleclick.net/gampad/clk?id=124407151&iu=/4140/ostg.clktrk
_______________________________________________
GeoTools-Devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geotools-devel
