Justin Deoliveira ha scritto:
Hi all,
I have a few issues with this patch which is supposed to address
performance with filters.
Ops, sorry...
1) It causes floating point strings to be truncated to an integer. The
following simple test case illustrates:
Literal l = ff.literal("0.1");
Object o = l.evaluate(null);
assertEquals( "0.1", o.toString() );
Darn...
2) Many of the changes are simply reformats, which make it hard to
figure out exactly what was changed.
Sorry about that but... looking at the diff using Eclipse I don't see
any reformat, just a clean patch (at least in LiteralExpressionImpl
class). Tried both on trunk and 2.4.x, attaching the result of
">svn diff src/main/java/org/geotools/filter/LiteralExpressionImpl.java
-r 28168:29061 > patch.diff". I don't see reformats there? (this
is from trunk).
Same goes for Value, I see a one line change.
May it be that the platform difference makes it show a different diff
on your machine (one with newlines changed)?
I thought the properties we have set in our svn clients prevented this
kind of issue?
(http://docs.codehaus.org/display/GEOT/2.4+Using+Subversion)
This is a bit of a serious bug imho. It means that whenever the literal
is evaluated without a class passed in (which is more often then not)
the values is truncated to an integer. One serious consequence in
GeoServer is the inability to use floating point numbers in SLD filters.
Regardless, the offending code is in LiteralExpressionImpl:
Class[] contexts = new Class[] {Integer.class, BigInteger.class,
Double.class};
for (int i = 0; i < contexts.length && parsedValue == null; i++) {
parsedValue = v.value(contexts[i]);
}
To try and guess the return type is unsafe imho.
As stated in the method's comment it's what the client
usage is expecting. The client does not provide a context to start
with, and it's expecting evaluate to do a proper guess, only because
that guess was once done in the SLD parser (in 2.3.x times).
Have a look at the old version of the code (the one that was there
before my patch):
public Object evaluate(Object feature) {
//hrm. Well, now that' we're always storing the internals of
//Literals as strings, we need to be slightly smart about how we
//return what's inside. Some (err, lots) of code relies on this
//method to return an instance of the correct TYPE. I guess we
should
//try and be somewhat smart about this.
//ASSERTION: literal is always a string.
if (literal == null || literal.getClass() != String.class) {
return literal;
}
String s = (String)literal;
try {
return new Integer(s);
} catch (Exception e) {
}
try {
return new Double(s);
} catch (Exception e) {
}
try {
return new BigInteger(s);
} catch (Exception e) {
}
return literal;
}
An alternative solution would be to just return the string as is, and
hunt down and fix every point in the code that's using evaluate(feature)
without providing a context and see if a legitimate context can be
provided instead.
If the converter api
guaranteed that an exception would be thrown in the case of going from a
floating point string to an integer then maybe... but it does not, nor
am i sure it should.
What's breaking that code is that the converter string -> integer
is returning a not null value for something that's not a clean
conversion. That is:
Converters.convert("2.1", Integer.class)
should return null, not "new Integer(2)" (imho).
I am also confused, because the jira issues refers to a slow down due to
converters... but this code is still going through the converter pipeline.
The jira issue does not state that's the cause, it just suggests it
might be: "Converters are the most likely cause, thought I did not do
any profile to check that."
I did not add any statement to confirm they were... in fact, they were
not, see the rest of the mail.
Regardless, if the point here is performance i suggest we do the following:
I suggest the following change:
try {
//try integer
parsedValue = Integer.parseInt( literal );
}
catch( NumberFormatException e ) {
//try double
try {
parsedValue = Double.parseDouble( literal );
}
catch( NumberFormatException e ) {
//try big decimal
try {
parsedValue = new BigDecimal( literal );
}
catch( NumberFormatException e ) {
//give up
}
}
}
This would be very similar to the original code, and is the reason for
its slowness: throwing and catching exceptions in a tight loop
(the evaluation of those literals is in the rendering loop)
is more expensive than going thru the converters.
The old 2.3.x times code did not do that, it stored stuff directly
as Double/Integer/... because the parsers tried to figure out
what the parsed thing was. But as we all know, this had other
problems.
Long story short, if you apply the patch you're proposing, you'll
restore correctness and have performance seriously regress again.
A different approach could be to apply just the double converter,
if the number happens to be an integer (no decimal part) then compare
with the integer max and min int, if it's in range, turn it into
an Integer, if out of range, turn it into a BigInteger.
How does this sound?
Cheers
Andrea
Index: src/main/java/org/geotools/filter/LiteralExpressionImpl.java
===================================================================
--- src/main/java/org/geotools/filter/LiteralExpressionImpl.java
(revisione 28168)
+++ src/main/java/org/geotools/filter/LiteralExpressionImpl.java
(revisione 29061)
@@ -37,6 +37,9 @@
/** Holds a reference to the literal. */
private Object literal = null;
+
+ /** The converted value guessed inside evaluate(Feature) **/
+ private Object parsedValue = null;
/**
* Constructor with literal.
@@ -215,26 +218,22 @@
//try and be somewhat smart about this.
//ASSERTION: literal is always a string.
- if (literal == null || literal.getClass() != String.class) {
- return literal;
- }
- String s = (String)literal;
- try {
- return new Integer(s);
- } catch (Exception e) {
- }
+ if(parsedValue != null)
+ return parsedValue;
- try {
- return new Double(s);
- } catch (Exception e) {
- }
-
- try {
- return new BigInteger(s);
- } catch (Exception e) {
- }
-
- return literal;
+ if (literal == null || !(literal instanceof String)) {
+ parsedValue = literal;
+ } else {
+ String s = (String) literal;
+ Value v = new Value(s);
+ Class[] contexts = new Class[] {Integer.class, BigInteger.class,
Double.class};
+ for (int i = 0; i < contexts.length && parsedValue == null; i++) {
+ parsedValue = v.value(contexts[i]);
+ }
+ if(parsedValue == null)
+ parsedValue = literal;
+ }
+ return parsedValue;
}
public Object evaluate(Object feature, Class context) {
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference
Don't miss this year's exciting event. There's still time to save $100.
Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
Geotools-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geotools-devel
