Re: [Geotools-devel] [Geoserver-devel] How to use "anonymous" OGC Filter Functions

[Gabriel R. <gr...@op...>]

Hi Andrea,
thanks for the review.

I understand all of that. InternalVolatileFunction is an abstract base
class, and just chose the default implementation of duplicate to
return this. Another alternative is not to implement it at all and
force subclasses to do so.

Note however, in my mind the duplicate(Expression...) method is just a
factory method to overcome the deviation from SPI in InternalFunction.
It shall not be responsible of duplicating its arguments in any way,
that's what DuplicatingFilterVisitor does, and calls
duplicate(Expression...) with the already transformed arguments.
Mentioning this just because I can't be sure what your expectations
were from your comments.

So, as I understand it, whomever is going to implement
InternalFunction has to implement duplicate() so that the new instance
works upon the provided arguments.
VolatileInternalFunction is just an abstract base class for the common
case where you just want to implement evaluate(Object), but can also
override duplicate(Expression...) if appropriate (i.e. the
implementation do work with Expression arguments).

That said I've not a strong position on leaving that specific method
implemented that way by default, and possibly document better that you
should override it as appropriate, or leave it unimplemented.
Any preference? or there's anything else that I'm missing?

Cheers,
Gabriel
On Wed, Jul 18, 2012 at 4:35 AM, Andrea Aime
<and...@ge...> wrote:
> On Tue, Jul 17, 2012 at 3:58 PM, Gabriel Roldan <gr...@op...> wrote:
>>
>> Patch attached to <http://jira.codehaus.org/browse/GEOT-4205>
>>
>> Please review and comment at your earliest convenience.
>
>
> Seems good, but the implementation of the InternalVolatileFunction seems
> broken to me here:
>
> /**
>      * @see
> org.opengis.filter.expression.InternalFunction#duplicate(org.opengis.filter.expression.Expression[])
>      */
>     @Override
>     public InternalFunction duplicate(Expression... parameters) {
>         return this;
>     }
>
> Say that foo is an internal volatile function, and you have foo(5 + 7),
> the simplifier can legitimately turn 5 +7 into 12 and call duplicate
> with just Literal(12) as the parameter, and you are going to ignore that.
>
> Other duplicators we have around do reprojection of geometries in spatial
> filters and the like, and they alter the geometry literals and wrap
> functions
> with on the fly reprojectors, if one of this modification happens to hit
> a parameter of your volatile internal function the above will simply ignore
> it,
> which introduces a bug.
>
> The duplicate method should be implemented function by function returning
> a copy of the function with the appropriate parameters.
>
> If the catalog related functions are parameter-less and you know that you
> should probably create a base class that's specific to those, and one that
> throws an illegal argument exception if the parameter list passed to
> duplicate
> is not empty
>
> 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 962313
> mob:   +39  339 8844549
>
> http://www.geo-solutions.it
> http://twitter.com/geosolutions_it
>
> -------------------------------------------------------
>



-- 
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.