[gdal-dev] OGR SQL implicit conversions

[Tamas Szekeres]

Hi Even,

Thanks for the valuable comments, I've modified the patch in accordance with
your suggestions.
http://trac.osgeo.org/gdal/attachment/ticket/4259/swq_op_general.cpp.patch

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.

Best regards,

Tamas



2011/9/23 Even Rouault <even.rouault at mines-paris.org>

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.osgeo.org/pipermail/gdal-dev/attachments/20110926/bd57257d/attachment.html