I think the answer will be to measure. As for the "cache" - I think we should
try the cache first; and if it works fine; if not we go and try the normal
technique (short listing accessors that canHandle; and then evaulating them)
Jody
On 08/09/2010, at 3:51 AM, Niels wrote:
> However, looking at it, there is another issue.
>
> AttributeExpressionImpl currently cashes its latest PropertyAccessor for
> performance reasons (as scanning the registry is very expensive).
>
> You can't do that anymore in the same way when we need to try multiple
> property accessors. We couldn't keep a cashe of a limited list of property
> accessors, because we loose the assumption that if we have one that 'can
> handle' the object, we don't need to try other ones anymore, we still need to
> check all the other ones.
>
> It is possible that we loose more performance this way, than by what I
> proposed earlier.
> This is a trade-off decision, but I am not sure which one is worst?
>
> Niels
>
> On 08/09/10 08:56, Niels wrote:
>>
>> I think your proposal is the most straight-forward way to solve the issue
>> without giving in on performance.
>>
>> However, the property accessor shouldn't return null but throw an exception
>> if the property doesn't exist, because there still should be a difference
>> between an actual null value and a non existing property.
>>
>> Niels
>>
>> On 07/09/10 20:04, Jody Garnett wrote:
>>>
>>> Hi Niels; we really need to keep discussion on the email list (as while my
>>> opinions may be helpful - they may not be correct).
>>> You may indeed have found a hole in the api; performing the work in
>>> canHandle method may still not be correct.
>>>
>>> Here is another idea; modify the look up code to go through all the
>>> property accessors; and short list XPathPropertyAccessor and
>>> ZPathPropertyAccessor. And then it can try both of them; and then if one of
>>> them returns non null...
>>>
>>> Jody
>>>
>>>
>>> On 07/09/2010, at 8:28 AM, Niels wrote:
>>>
>>>> Jody,
>>>>
>>>> I am still wondering about this.
>>>>
>>>> For example, say someone adds another PropertyAccessor, let's call it
>>>> ZPropertyAccessor.
>>>> It might come later in the list than XPathPropertyAccessor.
>>>>
>>>> The user specifies an attribute that can be evaluated by
>>>> ZPropertyAccessor. However, the string can also be interpreted as a valid
>>>> x-path, but it can't be evaluated as a property by the
>>>> XPathPropertyAccessor.
>>>>
>>>> The XPathPropertyAccessor will say "yes, I can handle this". But throw an
>>>> error as soon as it tries to evaluate it.
>>>>
>>>> ZPropertyAccessor will never be used. This will basically happen to any
>>>> accessor after XPathPropertyAccessor.
>>>>
>>>> That is why I think that, despite the performace issue,
>>>> XPathPropertyAccessor does have to check whether it can actually evaluate
>>>> the property in the canHandle method.
>>>>
>>>> What do you think?
>>>>
>>>> Thanks
>>>> Niels
>>>>
>>>> On 03/09/10 13:46, Niels wrote:
>>>>>
>>>>> Yes, you have a good point there about the performance because otherwise
>>>>> canHandle and evaluate are basically doing two times the same thing (the
>>>>> only way to check if the x-path points to a valid property is by trying
>>>>> to evaluate it).
>>>>>
>>>>> However, considering the API I'm not sure if you're right. The API for
>>>>> canHandle says "Can be used to perform checks against schema to ensure
>>>>> that the propery accessor will actually work with the provided instance.
>>>>> " SimpleFeaturePropertyAccessor does return canHandle=false for
>>>>> non-existing properties.
>>>>>
>>>>> The problem is: What if other property accessors are added in the future?
>>>>> They will never even be used if xpathpropertyaccessor comes before them
>>>>> in the list, returning an exception when maybe this other property
>>>>> accessor could still do the job. I think that is a theoretical
>>>>> possibility?
>>>>>
>>>>> Niels
>>>>>
>>>>> On 03/09/10 13:41, Jody Garnett wrote:
>>>>>>
>>>>>> Sounds good; one clarification for performance considerations.
>>>>>>
>>>>>> I am not sure the API will let you check the x-path against the feature
>>>>>> or feature type; the canHandle method is supposed to be fast; and should
>>>>>> just check that the xpath expression is valid.
>>>>>>
>>>>>> You will need to check that the xpath points to a property when the
>>>>>> expression is evaluated; at that point you could throw an exception (or
>>>>>> otherwise fail).
>>>>>>
>>>>>> Jody
>>>>>>
>>>>>> On 03/09/2010, at 3:11 PM, Niels wrote:
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Thanks for the advice.
>>>>>>>
>>>>>>> So my plan of attack will be:
>>>>>>>
>>>>>>> 1. Implement the 'canHandle' of class 'XPathPropertyAcessor' to make it
>>>>>>> return false when the x-path doesn't resolve to a property
>>>>>>> 2. Throw an exception in AttributeExpressionImpl.evaluate if no
>>>>>>> property accessor can be found.
>>>>>>> 3. Go through all the geotools test cases to be compatible with the
>>>>>>> changes.
>>>>>>>
>>>>>>> regards,
>>>>>>> Niels
>>>>>>>
>>>>>>> On 03/09/10 11:40, Jody Garnett wrote:
>>>>>>>>
>>>>>>>> I expect that JD (ie Justin's) comment was with respect to preserving
>>>>>>>> compatibility with the existing test cases when transitioning to the
>>>>>>>> GeoTools 2.3 Filter implementation (ie with Property Accessors).
>>>>>>>>
>>>>>>>> Now that the transition is mostly completed; throwing a good
>>>>>>>> exception is possible (but some work will be needed to update test
>>>>>>>> cases).
>>>>>>>>
>>>>>>>> This GeoTools 2.3 Filter transition is our oldest "technical debt") in
>>>>>>>> the library; there are a number of test cases that make use of the the
>>>>>>>> FilterImpl method directly (rather than using filter factory).
>>>>>>>>
>>>>>>>> Jody
>>>>>>>>
>>>>>>>> On Fri, Sep 3, 2010 at 1:36 PM, Jody Garnett <[email protected]>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> I think you could throw an exception there; and update the geotools
>>>>>>>>> test cases.
>>>>>>>>> Jody
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Thu, Sep 2, 2010 at 4:02 PM, Niels <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> I am a new geotools developer who just started working on app-schema
>>>>>>>>>> for
>>>>>>>>>> CSIRO last week.
>>>>>>>>>> I was asked to look at this problem:
>>>>>>>>>> http://jira.codehaus.org/browse/GEOT-3066 ("No error is returned if
>>>>>>>>>> I gave
>>>>>>>>>> an invalid column name in app-schema mapping file").
>>>>>>>>>>
>>>>>>>>>> I discovered the issue is intertwined with other parts of the
>>>>>>>>>> geotools code.
>>>>>>>>>> The issue is in the evaluation of expressions, more specifically in
>>>>>>>>>> the
>>>>>>>>>> AttributeExpressionImpl class. The evaluate method will return a
>>>>>>>>>> null value
>>>>>>>>>> when it cannot evaluate the attribute expression. This makes it
>>>>>>>>>> impossible
>>>>>>>>>> for App-schema to distinguish between an actual attribute with a
>>>>>>>>>> real null
>>>>>>>>>> value and a non-existing attribute. I think an evaluation method
>>>>>>>>>> should in
>>>>>>>>>> principle distinguish between an expression that cannot be evaluated
>>>>>>>>>> and an
>>>>>>>>>> expression that evaluates to a null value. However, there are
>>>>>>>>>> explicit
>>>>>>>>>> comments in the code that say this would cause issues with backward
>>>>>>>>>> compatibility:
>>>>>>>>>>
>>>>>>>>>> public Object evaluate(Object obj, Class target) {
>>>>>>>>>> PropertyAccessor accessor = getPropertyAccessor(obj, target);
>>>>>>>>>>
>>>>>>>>>> if ( accessor == null ) {
>>>>>>>>>> return null; //JD:not throwing exception to remain
>>>>>>>>>> backwards
>>>>>>>>>> compatabile, just returnign null
>>>>>>>>>> }
>>>>>>>>>> Object value = accessor.get( obj, attPath, target );
>>>>>>>>>>
>>>>>>>>>> if(target == null)
>>>>>>>>>> return value;
>>>>>>>>>>
>>>>>>>>>> return Converters.convert( value, target );
>>>>>>>>>>
>>>>>>>>>> }
>>>>>>>>>> Although I am not sure why, because the FunctionExpressionImpl class
>>>>>>>>>> does
>>>>>>>>>> throw exceptions when it cannot evaluate.
>>>>>>>>>> I considered bypassing the evaluate method altogether from
>>>>>>>>>> app-schema for
>>>>>>>>>> the mapping, but this is not a serious option because expressions
>>>>>>>>>> can be
>>>>>>>>>> quite complex, with functions etc. and all that functionality cannot
>>>>>>>>>> be
>>>>>>>>>> lost.
>>>>>>>>>>
>>>>>>>>>> I think the most optimal solution that respects backward
>>>>>>>>>> compatibility,
>>>>>>>>>> would be to introduce some flag somewhere that can turn error
>>>>>>>>>> checking on
>>>>>>>>>> for the evaluation of attribute expressions (but is off by default).
>>>>>>>>>> My
>>>>>>>>>> first idea to implement this is : creating an alternative
>>>>>>>>>> implementation of
>>>>>>>>>> AttributeExpression, and then passing an optional boolean value to
>>>>>>>>>> the CQL
>>>>>>>>>> compiler that specifies which attribute expression implementation to
>>>>>>>>>> use.
>>>>>>>>>> Ben Caradoc-Davies however suggested the use of the 'Hints', but I
>>>>>>>>>> am not
>>>>>>>>>> familiar yet with how this exactly works.
>>>>>>>>>>
>>>>>>>>>> What are your thoughts?
>>>>>>>>>>
>>>>>>>>>> One more thing, another issue that would need to be resolved is in
>>>>>>>>>> the
>>>>>>>>>> XPathPropertyAccessor class in the xml package.
>>>>>>>>>> When evaluating an attribute expression, an iteration through all
>>>>>>>>>> property
>>>>>>>>>> accessors determines which one to use.
>>>>>>>>>> When an invalid column name is specified, he will end up with
>>>>>>>>>> XPathPropertyAccessor as last choice. The problem is this code:
>>>>>>>>>> JXPathContext context =
>>>>>>>>>> JXPathContextFactory.newInstance().newContext(null, object);
>>>>>>>>>> context.setLenient(true);
>>>>>>>>>>
>>>>>>>>>> The Lenient property will make sure that no exception is thrown if
>>>>>>>>>> the xpath
>>>>>>>>>> cannot be resolved and a null is returned instead, again making it
>>>>>>>>>> impossible to distinguish between an error and a real null value. I
>>>>>>>>>> don't
>>>>>>>>>> know why it was chosen to do it this way?
>>>>>>>>>> That too would need to change.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> With Regards,
>>>>>>>>>> Niels
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Niels Charlier
>>>>>>>>>>
>>>>>>>>>> Software Engineer
>>>>>>>>>> CSIRO Earth Science and Resource Engineering
>>>>>>>>>> Phone: +61 8 6436 8914
>>>>>>>>>>
>>>>>>>>>> Australian Resources Research Centre
>>>>>>>>>> 26 Dick Perry Avenue, Kensington WA 6151
>>>>>>>>>>
>>>>>>>>>> ------------------------------------------------------------------------------
>>>>>>>>>> This SF.net Dev2Dev email is sponsored by:
>>>>>>>>>>
>>>>>>>>>> Show off your parallel programming skills.
>>>>>>>>>> Enter the Intel(R) Threading Challenge 2010.
>>>>>>>>>> http://p.sf.net/sfu/intel-thread-sfd
>>>>>>>>>> _______________________________________________
>>>>>>>>>> Geotools-devel mailing list
>>>>>>>>>> [email protected]
>>>>>>>>>> https://lists.sourceforge.net/lists/listinfo/geotools-devel
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Niels Charlier
>>>>>>>
>>>>>>> Software Engineer
>>>>>>> CSIRO Earth Science and Resource Engineering
>>>>>>> Phone: +61 8 6436 8914
>>>>>>>
>>>>>>> Australian Resources Research Centre
>>>>>>> 26 Dick Perry Avenue, Kensington WA 6151
>>>>>>> ------------------------------------------------------------------------------
>>>>>>> This SF.net Dev2Dev email is sponsored by:
>>>>>>>
>>>>>>> Show off your parallel programming skills.
>>>>>>> Enter the Intel(R) Threading Challenge 2010.
>>>>>>> http://p.sf.net/sfu/intel-thread-sfd_______________________________________________
>>>>>>> Geotools-devel mailing list
>>>>>>> [email protected]
>>>>>>> https://lists.sourceforge.net/lists/listinfo/geotools-devel
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Niels Charlier
>>>>>
>>>>> Software Engineer
>>>>> CSIRO Earth Science and Resource Engineering
>>>>> Phone: +61 8 6436 8914
>>>>>
>>>>> Australian Resources Research Centre
>>>>> 26 Dick Perry Avenue, Kensington WA 6151
>>>>
>>>>
>>>> --
>>>> Niels Charlier
>>>>
>>>> Software Engineer
>>>> CSIRO Earth Science and Resource Engineering
>>>> Phone: +61 8 6436 8914
>>>>
>>>> Australian Resources Research Centre
>>>> 26 Dick Perry Avenue, Kensington WA 6151
>>>
>>
>>
>> --
>> Niels Charlier
>>
>> Software Engineer
>> CSIRO Earth Science and Resource Engineering
>> Phone: +61 8 6436 8914
>>
>> Australian Resources Research Centre
>> 26 Dick Perry Avenue, Kensington WA 6151
>
>
> --
> Niels Charlier
>
> Software Engineer
> CSIRO Earth Science and Resource Engineering
> Phone: +61 8 6436 8914
>
> Australian Resources Research Centre
> 26 Dick Perry Avenue, Kensington WA 6151
> ------------------------------------------------------------------------------
> This SF.net Dev2Dev email is sponsored by:
>
> Show off your parallel programming skills.
> Enter the Intel(R) Threading Challenge 2010.
> http://p.sf.net/sfu/intel-thread-sfd_______________________________________________
> Geotools-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/geotools-devel
------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:
Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd
_______________________________________________
Geotools-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geotools-devel
