Hi Even,<div><br></div><div>Thanks for the valuable comments, I've modified the patch in accordance with your suggestions.</div><div><a href="http://trac.osgeo.org/gdal/attachment/ticket/4259/swq_op_general.cpp.patch">http://trac.osgeo.org/gdal/attachment/ticket/4259/swq_op_general.cpp.patch</a></div>
<div><br></div><div>Let me know if you find something else that can be changed. I've just added a warning if the conversion fails, but I'm not quite sure if we can easily generate errors at this state of the processing.</div>
<div><br></div><div>Best regards,</div><div><br></div><div>Tamas</div><div><br></div><div><br><br><div class="gmail_quote">2011/9/23 Even Rouault <span dir="ltr"><<a href="mailto:even.rouault@mines-paris.org">even.rouault@mines-paris.org</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">Le mercredi 21 septembre 2011 19:20:17, Tamas Szekeres a écrit :<br>
<div class="im">> Hi Devs,<br>
><br>
> We have some problems due to the recent changes in the SQL expression<br>
> parser which is related to the change in the implicit type conversion<br>
> behaviour.<br>
><br>
> Formerly we could safely use the following statement:<br>
><br>
> select * from mytable where ogr_fid in ('1100','1101','1102')<br>
><br>
> If the data type of ogr_fid is an integer or float then the expression<br>
> evaluator has converted the string constants to numeric values implicitly.<br>
><br>
> As of 1.8 the parser reports an error due to the incompatible types in the<br>
> expression.<br>
><br>
</div>> I've added a ticket #4259 <<a href="http://trac.osgeo.org/gdal/ticket/4259" target="_blank">http://trac.osgeo.org/gdal/ticket/4259</a>> related<br>
<div class="im">> to this problem including a patch to handle these string to numeric<br>
> conversions.<br>
<br>
</div>You didn't mention that the patch only applies to the comparison operators (<br>
>, >=, <, <=, =, <>, IN and BETWEEN ), which I think is an appropriate<br>
restriction. I was a bit skeptical at the beginning if it would also apply to<br>
+, - operators for example where it is not obvious in which way the conversion<br>
should be done.<br>
<br>
I have done a bit of testing with PostgreSQL, MySQL and SQLite and, and they<br>
indeed do the conversions you suggest.<br>
<br>
I've noticed that PostgreSQL issues an error when the conversion fails because<br>
the string constant isn't a numeric value.<br>
<br>
$ ogrinfo pg:dbname=autotest -sql "select * from poly where eas_id = 'a'"<br>
INFO: Open of `pg:dbname=autotest'<br>
using driver `PostgreSQL' successful.<br>
ERROR 1: ERROR: invalid input syntax for double precision type : « a »<br>
LINE 1: ...cuteSQLCursor CURSOR for select * from poly where eas_id = 'a'...<br>
<br>
MySQL is much more silent about failed conversions however. With the MySQL OGR<br>
driver, nothing is emitted, but in the mysql console client, I could see a<br>
difference :<br>
<br>
mysql> select * from poly where eas_id = 'a';<br>
Empty set, 10 warnings (0.00 sec)<br>
<br>
sqlite is completely silent in similar situations.<br>
<br>
Personnaly, I think it would be appropriate for the OGR SQL engine to also<br>
emit an error/warning for failed conversions (or perhaps just a CPLDebug() if<br>
we feel that there's no reason to be too verbose in such situations), but<br>
obviously there doesn't seem to be a consensus on that among SQL vendors.<br>
<br>
I believe this can be detected with something like :<br>
<br>
val = strtod(pszVal, &endptr);<br>
if (endptr != pszVal + strlen(pszVal))<br>
{<br>
CPLError(...)<br>
}<br>
<br>
<br>
As far as the proposed patch is concerned, a bit of comment in the new<br>
function to explain shortly that it converts string constants to float<br>
constants when there is a mix of arguments of type numeric and string. In<br>
which case, integer constants will also be promoted to float. But that last<br>
part happens to be what SWQAutoPromoteIntegerToFloat() does already, so<br>
perhaps you could drop that part and just call SWQAutoConvertStringToNumeric()<br>
before SWQAutoPromoteIntegerToFloat(), so that<br>
SWQAutoConvertStringToNumeric() only does what its name suggests (perhaps<br>
renaming it to SWQAutoConvertStringToFloat() would be more consistant by the<br>
way) ? I have tested that quickly and it seems to work, but more extensive<br>
testing would be welcome.<br>
<br>
On a stylistic note, I've had a hard time figuring out the operator precedence<br>
when reading the test :<br>
<br>
if( (eArgType == SWQ_STRING<br>
&& (poSubNode->field_type == SWQ_INTEGER<br>
|| poSubNode->field_type == SWQ_FLOAT)) ||<br>
(eArgType == SWQ_INTEGER || eArgType == SWQ_FLOAT)<br>
&& (poSubNode->field_type == SWQ_STRING) )<br>
<br>
So, I'd suggest to move the ( parenthesis of the last line on the line before<br>
(please check that it was indeed your intent) :<br>
<br>
if( (eArgType == SWQ_STRING<br>
&& (poSubNode->field_type == SWQ_INTEGER<br>
|| poSubNode->field_type == SWQ_FLOAT)) ||<br>
((eArgType == SWQ_INTEGER || eArgType == SWQ_FLOAT)<br>
&& poSubNode->field_type == SWQ_STRING) )<br>
<br>
Extending ogr_sql_rfc28.py with a new test for the new behaviour would also be<br>
great.<br>
<br>
So, all in all, I'm in favor of your approach and I'd suggest you should go<br>
ahead with it<br>
<div class="im"><br>
><br>
> However I'm a bit uncertain whether some other conversions should also be<br>
> supported or not. Having a larger number of conversion rules we might<br>
> probably think about rewriting the current approach by using a more generic<br>
> implementation.<br>
><br>
> Any ideas?<br>
<br>
</div>Not really...<br>
<br>
><br>
><br>
> Best regards,<br>
><br>
> Tamas<br>
</blockquote></div><br></div>