Opened 11 years ago

Closed 11 years ago

Last modified 9 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 11 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 by tamas, 11 years ago

Patch have been added to provide a string to numeric conversion

by tamas, 11 years ago

Attachment: swq_op_general.cpp.patch added

comment:2 by Even Rouault, 11 years ago

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.

in reply to:  2 comment:3 by tamas, 11 years ago

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 by Even Rouault, 11 years ago

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

in reply to:  4 comment:5 by tamas, 11 years ago

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 by Even Rouault, 11 years ago

OK

comment:7 by tamas, 11 years ago

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 by Even Rouault, 11 years ago

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 by tamas, 11 years ago

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

in reply to:  4 ; comment:10 by tamas, 11 years ago

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?

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

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

That's probably worth a new ticket

comment:12 by Even Rouault, 11 years ago

Somehow related ticket #4321 about joining columns of different types

comment:13 by Even Rouault, 11 years ago

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 by Even Rouault, 9 years ago

Milestone: 1.8.2

Milestone 1.8.2 deleted

Note: See TracTickets for help on using tickets.