Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#4259 closed defect (fixed)

OGR SQL implicit conversion from string to numeric

Reported by: tamas Owned by: tamas
Priority: normal Milestone:
Component: default Version:
Severity: normal Keywords:
Cc:

Description

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.

Attachments (1)

swq_op_general.cpp.patch (3.1 KB) - added by tamas 6 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 6 years ago by tamas

Patch have been added to provide a string to numeric conversion

Changed 6 years ago by tamas

Attachment: swq_op_general.cpp.patch added

comment:2 Changed 6 years ago by Even 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.

comment:3 in reply to:  2 Changed 6 years ago by 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.

comment:4 Changed 6 years ago by Even 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

comment:5 in reply to:  4 Changed 6 years ago by 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.

comment:6 Changed 6 years ago by Even Rouault

OK

comment:7 Changed 6 years ago by 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...

comment:8 Changed 6 years ago by Even 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) )

comment:9 Changed 6 years ago by tamas

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

comment:10 in reply to:  4 ; Changed 6 years ago by 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?

comment:11 in reply to:  10 Changed 6 years ago by Even Rouault

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

That's probably worth a new ticket

comment:12 Changed 6 years ago by Even Rouault

Somehow related ticket #4321 about joining columns of different types

comment:13 Changed 6 years ago by Even Rouault

Resolution: fixed
Status: newclosed

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

comment:14 Changed 5 years ago by Even Rouault

Milestone: 1.8.2

Milestone 1.8.2 deleted

Note: See TracTickets for help on using tickets.