[Geoserver-devel] filter to sql encoding issue with Add

Justin_Deoliveira · June 11, 2008, 5:28am

Hi all,

While running wfs 1.1 cite tests on trunk I ran into the following issue. Consider the filter:

<ogc:Filter>
   <ogc:PropertyIsEqualTo>
     <ogc:PropertyName>sf:intProperty</ogc:PropertyName>
     <ogc:Add>
        <ogc:PropertyName>sf:decimalProperty</ogc:PropertyName>
        <ogc:Literal>149.97</ogc:Literal>
     </ogc:Add>
   </ogc:PropertyIsEqualTo>
</ogc:Filter>

When encoded as a predicate in sql:

WHERE "intProperty" = "decimalProperty" + 149

Now... the reason this occurs is because the literal 149.97 is evaluated in the context of an integer and is truncated.

I can think of a couple of solutions:

1. When encoding a binary expression like "Add" checking for the case of a literal being added to a property. And instead of using the context passed in while encoding the literal part, use the type of property operand.

2. Ignore the context when encoding an Add and just encode everything "raw" letting the database do the conversions.

However there is one problem, in both cases the "solution" kills the use of an index on the column being compared to. So... two more choices:

1. Leave things as is and ignore the crazy people that want to do this.

2. Implement the solution and tell people that if they do this they risk a performance hit.

I will note that 1. will leave us non cite compliant...

Thoughts? On the bright side this was the only cite failure on trunk :).

--
Justin Deoliveira
The Open Planning Project
jdeolive@anonymised.com

Jody_Garnett · June 11, 2008, 6:15am

Justin Deoliveira wrote:

Hi all,

While running wfs 1.1 cite tests on trunk I ran into the following issue. Consider the filter:

<ogc:Filter>
   <ogc:PropertyIsEqualTo>
     <ogc:PropertyName>sf:intProperty</ogc:PropertyName>
     <ogc:Add>
        <ogc:PropertyName>sf:decimalProperty</ogc:PropertyName>
        <ogc:Literal>149.97</ogc:Literal>
     </ogc:Add>
   </ogc:PropertyIsEqualTo>
</ogc:Filter>

When encoded as a predicate in sql:

WHERE "intProperty" = "decimalProperty" + 149

nasty! So we almost need two rounds of "type normalization"...
If we treat "Add" as a separate negotiation; expression1 is the only one with a nice strong type ..

<ogc:Add>
<ogc:PropertyName>sf:decimalProperty</ogc:PropertyName>
<ogc:Literal>149.97</ogc:Literal>
</ogc:Add>

So this could end up adding two doubles ... resulting in a double

Then the propertyEquals check can do its own attempt a negotiate between a intProperty and double; and end up comparing two ints...

Now... the reason this occurs is because the literal 149.97 is evaluated in the context of an integer and is truncated.
I can think of a couple of solutions:

1. When encoding a binary expression like "Add" checking for the case of a literal being added to a property. And instead of using the context passed in while encoding the literal part, use the type of property operand.

Yeah; that is my though; so we take the type of either expression1 or expression2 (if either is a Propertyname); and handle the conversion from the answer to the target type as a seperate step?

2. Ignore the context when encoding an Add and just encode everything "raw" letting the database do the conversions.

That is an option; encoding into SQL take the context into account if it helps (say for Timestamp) and ignore in cases where it does not like Add.

However there is one problem, in both cases the "solution" kills the use of an index on the column being compared to.

Does SQL have a function to "cast" a value into a known type?

So... two more choices:

1. Leave things as is and ignore the crazy people that want to do this.

2. Implement the solution and tell people that if they do this they risk a performance hit.

I will note that 1. will leave us non cite compliant...

CITE has a test for this?

Thoughts? On the bright side this was the only cite failure on trunk :).

That is great news - well done.

So this failure amounts to us being smarter than average bear. Or at least us over thinking the problem ... do we have the option of telling them the CITE test is bad? Or are we just trying to be wrong faster...

Jody

Andrea_Aime3 · June 11, 2008, 11:54am

Justin Deoliveira ha scritto:

Hi all,

While running wfs 1.1 cite tests on trunk I ran into the following issue. Consider the filter:

<ogc:Filter>
   <ogc:PropertyIsEqualTo>
     <ogc:PropertyName>sf:intProperty</ogc:PropertyName>
     <ogc:Add>
        <ogc:PropertyName>sf:decimalProperty</ogc:PropertyName>
        <ogc:Literal>149.97</ogc:Literal>
     </ogc:Add>
   </ogc:PropertyIsEqualTo>
</ogc:Filter>

When encoded as a predicate in sql:

WHERE "intProperty" = "decimalProperty" + 149

Now... the reason this occurs is because the literal 149.97 is evaluated in the context of an integer and is truncated.

Where is the code that is doing that? SqlEncoder?
I believe the encoding code was modified so that the "best guess" from
the string was used instead of the context which, as you say, cannot
be really trusted to be a "rule" to follow.... oh but that was done
only in FilterToSql, not in SqlEncoder (see
http://jira.codehaus.org/browse/geot-1802… and I don't remember why
I did fix FilterToSql instead of SqlEncoder...)

Is the above a insert/update/delete or a select? The code path in
PostgisDataStore is different for the two at the moment, the code
that does write is using prepared statements, the one that does
reads is still not.

I can think of a couple of solutions:

1. When encoding a binary expression like "Add" checking for the case of a literal being added to a property. And instead of using the context passed in while encoding the literal part, use the type of property operand.

Imho we should pass the encoder function just a Number.class context,
that is, let it know we're doing math and not strings operations, but
let it figure out the best holding type automatically.

2. Ignore the context when encoding an Add and just encode everything "raw" letting the database do the conversions.

However there is one problem, in both cases the "solution" kills the use of an index on the column being compared to. So... two more choices:

1. Leave things as is and ignore the crazy people that want to do this.

2. Implement the solution and tell people that if they do this they risk a performance hit.

I believe encoding sql corresponding to the filter is more important
than speed (yes, correctness is more important than speed, even for me... but I want both to consider something worthwhile using )

It is good to encode 5.0 as 5 performance wise, but we cannot encode
5.1 as 5, if you follow me.

Cheers
Andrea

Justin_Deoliveira · June 11, 2008, 4:14pm

Andrea Aime wrote:

Justin Deoliveira ha scritto:

Hi all,

While running wfs 1.1 cite tests on trunk I ran into the following issue. Consider the filter:

<ogc:Filter>
   <ogc:PropertyIsEqualTo>
     <ogc:PropertyName>sf:intProperty</ogc:PropertyName>
     <ogc:Add>
        <ogc:PropertyName>sf:decimalProperty</ogc:PropertyName>
        <ogc:Literal>149.97</ogc:Literal>
     </ogc:Add>
   </ogc:PropertyIsEqualTo>
</ogc:Filter>

When encoded as a predicate in sql:

WHERE "intProperty" = "decimalProperty" + 149

Now... the reason this occurs is because the literal 149.97 is evaluated in the context of an integer and is truncated.

Where is the code that is doing that? SqlEncoder?
I believe the encoding code was modified so that the "best guess" from
the string was used instead of the context which, as you say, cannot
be really trusted to be a "rule" to follow.... oh but that was done
only in FilterToSql, not in SqlEncoder (see
http://jira.codehaus.org/browse/geot-1802… and I don't remember why
I did fix FilterToSql instead of SqlEncoder...)

Yeah its in FilterToSQL.

Is the above a insert/update/delete or a select? The code path in
PostgisDataStore is different for the two at the moment, the code
that does write is using prepared statements, the one that does
reads is still not.

I should note that I am seeing this is in the H2 module, and it is a select statement.

I can think of a couple of solutions:

1. When encoding a binary expression like "Add" checking for the case of a literal being added to a property. And instead of using the context passed in while encoding the literal part, use the type of property operand.

Imho we should pass the encoder function just a Number.class context,
that is, let it know we're doing math and not strings operations, but
let it figure out the best holding type automatically.

Ok.. at the moment it is getting passed a Long... so a number should fix it. I guess I need to track down where this is getting passed in from. I guess from the PropertyIs* methods which look at the type of the property and pass that as context. So will this be a special case of ensuring that when numeric values are being worked with the context should just be Number?

2. Ignore the context when encoding an Add and just encode everything "raw" letting the database do the conversions.

However there is one problem, in both cases the "solution" kills the use of an index on the column being compared to. So... two more choices:

1. Leave things as is and ignore the crazy people that want to do this.

2. Implement the solution and tell people that if they do this they risk a performance hit.

I believe encoding sql corresponding to the filter is more important
than speed (yes, correctness is more important than speed, even for me... but I want both to consider something worthwhile using )

Wow... the next thing you are going to tell me is that hell is freezing over ;).

It is good to encode 5.0 as 5 performance wise, but we cannot encode
5.1 as 5, if you follow me.

I do.

Cheers
Andrea

!DSPAM:4007,484fbcfa293607180515871!

--
Justin Deoliveira
The Open Planning Project
jdeolive@anonymised.com

Andrea_Aime3 · June 12, 2008, 8:12am

Justin Deoliveira ha scritto:
...

When encoded as a predicate in sql:

WHERE "intProperty" = "decimalProperty" + 149

Now... the reason this occurs is because the literal 149.97 is evaluated in the context of an integer and is truncated.

Where is the code that is doing that? SqlEncoder?
I believe the encoding code was modified so that the "best guess" from
the string was used instead of the context which, as you say, cannot
be really trusted to be a "rule" to follow.... oh but that was done
only in FilterToSql, not in SqlEncoder (see
http://jira.codehaus.org/browse/geot-1802… and I don't remember why
I did fix FilterToSql instead of SqlEncoder...)

Yeah its in FilterToSQL.

Is the above a insert/update/delete or a select? The code path in
PostgisDataStore is different for the two at the moment, the code
that does write is using prepared statements, the one that does
reads is still not.

I should note that I am seeing this is in the H2 module, and it is a select statement.

I can think of a couple of solutions:

1. When encoding a binary expression like "Add" checking for the case of a literal being added to a property. And instead of using the context passed in while encoding the literal part, use the type of property operand.

Imho we should pass the encoder function just a Number.class context,
that is, let it know we're doing math and not strings operations, but
let it figure out the best holding type automatically.

Ok.. at the moment it is getting passed a Long... so a number should fix it. I guess I need to track down where this is getting passed in from. I guess from the PropertyIs* methods which look at the type of the property and pass that as context. So will this be a special case of ensuring that when numeric values are being worked with the context should just be Number?

Hum, imho we should pass Number.class as the context in all cases where
we know a number is needed, so in Add, Divide, Multiply, Subtract,
should you ever be able to reach them without going thru PropertyIs*
(function arguments for example, not sure there are other cases).

A more centralized solution would be to treat numbers in a special
way in visit(Literal) and have the expression just figure out the best
representation for the number at hand?
This would be accomplished by simply adding an extra check before
trying to make the conversion:

if(target != null && !Number.isAssignableFrom(target))
literal = expression.evaluate(null, target);

An even better solution would be to change the converters so that
they do not try to coax 5.1 into 5, that is, allow the conversion
only when it's clean, and return null instead (in this case the
Which is something we already discussed in gt2-devel I believe, but I
can't find the thread anymore.
The forced casting that turns 149.97 into 149 is in NumericConverterFactory btw.

Cheers
Andrea

Andrea_Aime3 · June 12, 2008, 8:16am

Andrea Aime ha scritto:
...

An even better solution would be to change the converters so that
they do not try to coax 5.1 into 5, that is, allow the conversion
only when it's clean, and return null instead (in this case the
Which is something we already discussed in gt2-devel I believe, but I
can't find the thread anymore.

Ah, here is the discussion:

http://www.nabble.com/On-converters-and-target-class-usage-ts17105366.html#a17105366

the topic was abandoned out of frustration, the time I could have
spent fixing the issue went down solid into writing mails...
Cheers
Andrea

Justin_Deoliveira · June 12, 2008, 5:13pm

http://www.nabble.com/On-converters-and-target-class-usage-ts17105366.html#a17105366

the topic was abandoned out of frustration, the time I could have
spent fixing the issue went down solid into writing mails...

Ahhh... right. I have been playing with converters lately perhaps i will try to play with adding a "safe" path to the converters.

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

!DSPAM:4007,4850db85168565210051143!

--
Justin Deoliveira
The Open Planning Project
jdeolive@anonymised.com