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.roua...@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 >
_______________________________________________ gdal-dev mailing list gdal-dev@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/gdal-dev