From: Andrea A. <and...@ge...> - 2012-07-18 07:35:53
|
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 ------------------------------------------------------- |