Selon Tamas Szekeres <[email protected]>: > 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 <[email protected]> > > > 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 > > > _______________________________________________ gdal-dev mailing list [email protected] http://lists.osgeo.org/mailman/listinfo/gdal-dev
