[gdal-dev] OGR SQL implicit conversions

[Even Rouault]

Le mercredi 21 septembre 2011 19:20:17, Tamas Szekeres a écrit :
> Hi Devs,
> 
> We have some problems due to the recent changes in the SQL expression
> parser which is related to the change in the implicit type conversion
> behaviour.
> 
> Formerly we could safely use the following statement:
> 
> select * from mytable where ogr_fid in ('1100','1101','1102')
> 
> If the data type of ogr_fid is an integer or float then the expression
> evaluator has converted the string constants to numeric values implicitly.
> 
> As of 1.8 the parser reports an error due to the incompatible types in the
> expression.
> 
> I've added a ticket #4259 <http://trac.osgeo.org/gdal/ticket/4259> related
> to this problem including a patch to handle these string to numeric
> conversions.

You didn't mention that the patch only applies to the comparison operators ( 
>, >=, <, <=, =, <>, IN and BETWEEN ), which I think is an appropriate 
restriction. I was a bit skeptical at the beginning if it would also apply to 
+, - operators for example where it is not obvious in which way the conversion 
should be done.

I have done a bit of testing with PostgreSQL, MySQL and SQLite and, and they 
indeed do the conversions you suggest.

I've noticed that PostgreSQL issues an error when the conversion fails because 
the string constant isn't a numeric value.

$ ogrinfo pg:dbname=autotest -sql "select * from poly where eas_id = 'a'"
INFO: Open of `pg:dbname=autotest'
      using driver `PostgreSQL' successful.
ERROR 1: ERROR: invalid input syntax for double precision type : « a »
LINE 1: ...cuteSQLCursor CURSOR for select * from poly where eas_id = 'a'...

MySQL is much more silent about failed conversions however. With the MySQL OGR 
driver, nothing is emitted, but in the mysql console client, I could see a 
difference :

mysql> select * from poly where eas_id = 'a';
Empty set, 10 warnings (0.00 sec)

sqlite is completely silent in similar situations.

Personnaly, I think it would be appropriate for the OGR SQL engine to also 
emit an error/warning for failed conversions (or perhaps just a CPLDebug() if 
we feel that there's no reason to be too verbose in such situations), but 
obviously there doesn't seem to be a consensus on that among SQL vendors.

I believe this can be detected with something like :

val = strtod(pszVal, &endptr);
if (endptr != pszVal + strlen(pszVal))
{
	CPLError(...)
}


As far as the proposed patch is concerned, a bit of comment in the new 
function to explain shortly that it converts string constants to float 
constants when there is a mix of arguments of type numeric and string. In 
which case, integer constants will also be promoted to float. But that last 
part happens to be what SWQAutoPromoteIntegerToFloat() does already, so 
perhaps you could drop that part and just call SWQAutoConvertStringToNumeric() 
before SWQAutoPromoteIntegerToFloat(), so that 
SWQAutoConvertStringToNumeric() only does what its name suggests (perhaps 
renaming it to SWQAutoConvertStringToFloat() would be more consistant by the 
way) ? I have tested that quickly and it seems to work, but more extensive 
testing would be welcome.

On a stylistic note, I've had a hard time figuring out the operator precedence 
when reading the test :

        if( (eArgType == SWQ_STRING
            && (poSubNode->field_type == SWQ_INTEGER
                || poSubNode->field_type == SWQ_FLOAT)) || 
            (eArgType == SWQ_INTEGER || eArgType == SWQ_FLOAT)
            && (poSubNode->field_type == SWQ_STRING) )

So, I'd suggest to move the ( parenthesis of the last line on the line before 
(please check that it was indeed your intent) :

        if( (eArgType == SWQ_STRING
            && (poSubNode->field_type == SWQ_INTEGER
                || poSubNode->field_type == SWQ_FLOAT)) || 
            ((eArgType == SWQ_INTEGER || eArgType == SWQ_FLOAT)
            && poSubNode->field_type == SWQ_STRING) )

Extending ogr_sql_rfc28.py with a new test for the new behaviour would also be 
great.

So, all in all, I'm in favor of your approach and I'd suggest you should go 
ahead with it

> 
> However I'm a bit uncertain whether some other conversions should also be
> supported or not. Having a larger number of conversion rules we might
> probably think about rewriting the current approach by using a more generic
> implementation.
> 
> Any ideas?

Not really...

> 
> 
> Best regards,
> 
> Tamas