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
------------------------------------------------------------------------------
ThisSF.net <http://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 <http://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
