OGR SQL implicit conversion from string to numeric

[tamas]

Most SQL query implementations support implicit conversions between various types in the SQL expressions. For example we can safely use the following syntax:

select * from rivers where ogr_fid in ('1100','1101','1102')

If the data type of ogr_fid is an integer or float then the expression evaluator would convert the string constants to numeric values implicitly.

This conversion has been working until the new expression parser have was introduced in gdal 1.8. As of 1.8 the parser reports an error due to the incompatible types in the expression.

[tamas]

Patch have been added to provide a string to numeric conversion

[rouault]

Tamas, your patch looks good and my testing didn't show any issue, so I think you can just go ahead and commit it.

One small question however: why is "poSubNode->int_value = (int)poSubNode->float_value;" necessary ? If the node is declared as SWQ_FLOAT, no one should ever try to read the int_value field. For example, the swq_expr_node::swq_expr_node( double dfValueIn ) constructor only set the float_value field.

[tamas]

Replying to rouault:

Tamas, your patch looks good and my testing didn't show any issue, so I think you can just go ahead and commit it. One small question however: why is "poSubNode->int_value = (int)poSubNode->float_value;" necessary ? If the node is declared as SWQ_FLOAT, no one should ever try to read the int_value field. For example, the swq_expr_node::swq_expr_node( double dfValueIn ) constructor only set the float_value field.

Even, in some cases the field type is used instead of the node type (for an example see ogrfeaturequery.cpp line 296) In a particular case if ogr_fid is an integer field, we may have problems with the string to float conversion if we don't set int_value in addition.

[rouault]

Interesting, but I believe you've just found a bug in OGRFeatureQuery::EvaluateAgainstIndices?() that should be fixed (by testing the node type and doing the appropriate cast), and not in your new code. Because we currently have a bug *without* your new code :

ogr2ogr mypoly.shp ../autotest/ogr/data/poly.shp -sql "select cast(EAS_ID as integer) as EAS_ID_INT, * from poly"
ogrinfo mypoly.shp -sql "create index on mypoly using EAS_ID_INT"
ogrinfo mypoly.shp -where "EAS_ID_INT in (170)" -al --> works
ogrinfo mypoly.shp -where "EAS_ID_INT in (170.0)" -al --> doesn't work

[tamas]

I think the safest way is to leave the "poSubNode->int_value = (int)poSubNode->float_value;" assignment as it is, and provide a fix for the bug in addition.

[rouault]

OK

[tamas]

Applied in trunk (r23205).

I'm not sure whether to treat this one as a bug or new feature and include the change in the stable branch as well? Anyway this functionality have been working in the pre 1.8 versions before the new parser was added...

[rouault]

Being a regression fix and reasonably small and non-risky, it looks OK to me for 1.8 backporting.

(I've pushed r23210 /trunk/autotest/ogr/ogr_sql_test.py: Test implicit conversion from string to numeric (#4259) )

[tamas]

Added the fix (r23212) and the corresponding test case (r23213) in the stable branch.

[tamas]

Replying to rouault:

Interesting, but I believe you've just found a bug in OGRFeatureQuery::EvaluateAgainstIndices?() that should be fixed (by testing the node type and doing the appropriate cast), and not in your new code. Because we currently have a bug *without* your new code : {{{ ogr2ogr mypoly.shp ../autotest/ogr/data/poly.shp -sql "select cast(EAS_ID as integer) as EAS_ID_INT, * from poly" ogrinfo mypoly.shp -sql "create index on mypoly using EAS_ID_INT" ogrinfo mypoly.shp -where "EAS_ID_INT in (170)" -al --> works ogrinfo mypoly.shp -where "EAS_ID_INT in (170.0)" -al --> doesn't work }}}

Should we handle this problem within the scope of this issue?

[rouault]

Should we handle this problem within the scope of this issue?

That's probably worth a new ticket

[rouault]

Somehow related ticket #4321 about joining columns of different types

[rouault]

I close this ticket. I've created a separate ticket for the side issue mentionned above (bug in EvaluateAgainstIndices?()) : #4326

[rouault]

Milestone 1.8.2 deleted