[gdal-dev] OGR SQL implicit conversions

[Even Rouault]

Selon Tamas Szekeres <szekerest at gmail.com>:

> 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
>

Looks good to me.

> 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
> >
>