Bug in MsSql.cs?

Topics: General Topics, SharpMap v0.9 / v1.x, SharpMap v2.0
Oct 27, 2007 at 2:39 AM
I'm currently using SharpMap 0.9, although I checked to see if the source code for this section is similar in 2.0, and I believe it is. When I call SharpMap.Map.ZoomToExtends using the MsSql provider, I get the following exception:

System.InvalidCastException was unhandled
Message="Specified cast is not valid."
Source="SharpMap"
StackTrace:
at SharpMap.Data.Providers.MsSql.GetExtents() in C:\Documents and Settings\jnoring\My Documents\Visual Studio 2005\Projects\SharpMapDemo\SharpMap\Data\Providers\MsSql.cs:line 406
at SharpMap.Layers.VectorLayer.get_Envelope() in C:\Documents and Settings\jnoring\My Documents\Visual Studio 2005\Projects\SharpMapDemo\SharpMap\Layers\VectorLayer.cs:line 298
at SharpMap.Map.GetExtents() in C:\Documents and Settings\jnoring\My Documents\Visual Studio 2005\Projects\SharpMapDemo\SharpMap\Map\Map.cs:line 369
at SharpMap.Map.ZoomToExtents() in C:\Documents and Settings\jnoring\My Documents\Visual Studio 2005\Projects\SharpMapDemo\SharpMap\Map\Map.cs:line 190
at SharpMapDemo.MainForm.InitializeMap() in C:\Documents and Settings\jnoring\My Documents\Visual Studio 2005\Projects\SharpMapDemo\Form1.cs:line 99
at SharpMapDemo.MainForm..ctor() in C:\Documents and Settings\jnoring\My Documents\Visual Studio 2005\Projects\SharpMapDemo\Form1.cs:line 23
at SharpMapDemo.Program.Main() in C:\Documents and Settings\jnoring\My Documents\Visual Studio 2005\Projects\SharpMapDemo\Program.cs:line 17
at System.AppDomain.nExecuteAssembly(Assembly assembly, String[] args)
at System.AppDomain.ExecuteAssembly(String assemblyFile, Evidence assemblySecurity, String[] args)
at Microsoft.VisualStudio.HostingProcess.HostProc.RunUsersAssembly()
at System.Threading.ThreadHelper.ThreadStart_Context(Object state)
at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
at System.Threading.ThreadHelper.ThreadStart()

...the callstack at this point looks like:

> SharpMap.dll!SharpMap.Data.Providers.MsSql.GetExtents() Line 406 C#
SharpMap.dll!SharpMap.Layers.VectorLayer.Envelope.get() Line 298 + 0x13 bytes C#
SharpMap.dll!SharpMap.Map.GetExtents() Line 369 + 0x22 bytes C#
SharpMap.dll!SharpMap.Map.ZoomToExtents() Line 190 + 0x9 bytes C#
SharpMapDemo.exe!SharpMapDemo.MainForm.InitializeMap() Line 99 + 0x18 bytes C#
SharpMapDemo.exe!SharpMapDemo.MainForm.MainForm() Line 23 + 0x7 bytes C#
SharpMapDemo.exe!SharpMapDemo.Program.Main() Line 17 + 0x13 bytes C#

The specific line of code that's crashing is:

box = new SharpMap.Geometries.BoundingBox((float)dr0, (float)dr1, (float)dr2, (float)dr3);

I believe the crash is happening because of a boxing issue. The underlying value is of type double, but it's been cast to an object type. Attempting to then cast that to a float type results in the InvalidCastException because the value is boxed to a double, and needs to be unboxed to the same type. There are also some precision issues with casting from type double to float (as in, you loose it).

With that in mind, I'd suggesting changing the above code to something like:

if (typeof(float) == dr0.GetType())
box = new SharpMap.Geometries.BoundingBox((float)dr0, (float)dr1, (float)dr2, (float)dr3);
else if (typeof(double) == dr0.GetType())
box = new SharpMap.Geometries.BoundingBox((double)dr0, (double)dr1, (double)dr2, (double)dr3);
else
throw new InvalidCastException();


One final question--I'm starting a new project. Should we be using 2.0 beta, or is .9 good for now?
Oct 28, 2007 at 6:47 PM
The doc clearly states that you should be using double precision values in your database table. BoundingBox's constructor doesn't take floats.
Oct 29, 2007 at 9:38 AM
I'm affraid there's another one problem when I have a layer without any feature - this case is valid, isn't it?
Then
box = new SharpMap.Geometries.BoundingBox((float)dr0, (float)dr1, (float)dr2, (float)dr3);
will fail, because dr0 is null. Is there any reason why not return null in this case in this method?

if (dr0 != System.DBNull.Value || dr1 != System.DBNull.Value || dr2 != System.DBNull.Value || dr3 != System.DBNull.Value)
box = new SharpMap.Geometries.BoundingBox((double)dr0, (double)dr1, (double)dr2, (double)dr3);

And it's not bad idea to use TryParse :-)

Jirka Nouza
Oct 29, 2007 at 4:10 PM
Edited Oct 29, 2007 at 4:20 PM
The doc clearly states that you should be using double precision values in your database table. BoundingBox's constructor doesn't take floats.

OK, you "clearly" didn't read what I wrote. Let's try again. This is the line of code in SharpMap:

box = new SharpMap.Geometries.BoundingBox((float)dr[0], (float)dr[1], (float)dr[2], (float)dr[3]);

...this is not a line of code that I wrote; this is in MsSql.cs, which is a file in SharpMap. dr[2] is of type object, and the underlying type is double. And I'm aware that BoundingBox's constructor doesn't take floats, which makes this code obviously in error.

Please read carefully before responding to posts so I don't have to restate things that are obvious.
Oct 29, 2007 at 4:31 PM
Edited Oct 29, 2007 at 4:31 PM

jnouza wrote:
I'm affraid there's another one problem when I have a layer without any feature - this case is valid, isn't it?
Then
box = new SharpMap.Geometries.BoundingBox((float)dr0, (float)dr1, (float)dr2, (float)dr3);
will fail, because dr0 is null.


I don't think it would fail in the current codebase, actually, because:

using (SqlDataReader dr = command.ExecuteReader())
if (dr.Read())
{
box = new SharpMap.Geometries.BoundingBox((float)dr0, (float)dr1, (float)dr2, (float)dr3);
}

...so with an empty map, I'd assume dr.Read() would return false (which means MsSql.GetExtents() returns null) because there were no rows returned.

If the data is always of type double, then this code just needs to be changed to:

using (SqlDataReader dr = command.ExecuteReader())
if (dr.Read())
{
box = new SharpMap.Geometries.BoundingBox((double)dr0, (double)dr1, (double)dr2, (double)dr3);
}
Oct 30, 2007 at 6:29 AM
Sorry about my screwup on floats - it's been a while and I'm just trying to help.
The reason for using floats in the database (which actually clearly states to use 'real' for db type) is for size limitation and speed. The full accuracy is still maintained on the geometry. If you need something else, grab a copy of the class and create your own that uses doubles. Thats the neat thing about SharpMap: It's easily extensible with new datasources that suits your special needs.

How can those extent values be null? The query is looking for the min and max values of those columns.

Besides, MsSql should be considered outdated long ago by MsSqlSpatial. Any reason you are not using this instead?
Oct 30, 2007 at 2:03 PM
Edited Oct 30, 2007 at 2:04 PM

kidjan wrote:
...so with an empty map, I'd assume dr.Read() would return false (which means MsSql.GetExtents() returns null) because there were no rows returned.


Let'a assume this query:
SELECT Min(thegeomEnvelopeMinX) AS MinX, Min(thegeomEnvelopeMinY) AS MinY, Max(thegeomEnvelopeMaxX) AS MaxX, Max(thegeomEnvelopeMaxY) AS MaxY
FROM obcemcasti_eobce WHERE 1=2 -- or for. example WHERE AvgAge>100

And it returns:
MinX MinY MaxX MaxY
NULL NULL NULL NULL

Oct 30, 2007 at 2:06 PM

Besides, MsSql should be considered outdated long ago by MsSqlSpatial. Any reason you are not using this instead?


No special reason except of history. I'll try MsSqlSpatial data provider.
Oct 31, 2007 at 8:19 PM

jnouza wrote:

kidjan wrote:
...so with an empty map, I'd assume dr.Read() would return false (which means MsSql.GetExtents() returns null) because there were no rows returned.


Let'a assume this query:
SELECT Min(thegeomEnvelopeMinX) AS MinX, Min(thegeomEnvelopeMinY) AS MinY, Max(thegeomEnvelopeMaxX) AS MaxX, Max(thegeomEnvelopeMaxY) AS MaxY
FROM obcemcasti_eobce WHERE 1=2 -- or for. example WHERE AvgAge>100

And it returns:
MinX MinY MaxX MaxY
NULL NULL NULL NULL




It doesn't return MinX, MinY, MaxX, MaxY--it returns rows encapsulated inside of a SqlDataReader object. If the object has no rows (as it would if you added a clause like WHERE 1=2), then it isn't going to run the inner code. None of these queries would cause a crash, so far as I can see, because no rows would be returned, and this line:

box = new SharpMap.Geometries.BoundingBox((float)dr0, (float)dr1, (float)dr2, (float)dr3);

...is never hit.
Oct 31, 2007 at 8:34 PM

Odegaard wrote:
The reason for using floats in the database (which actually clearly states to use 'real' for db type) is for size limitation and speed. The full accuracy is still maintained on the geometry. If you need something else, grab a copy of the class and create your own that uses doubles. Thats the neat thing about SharpMap: It's easily extensible with new datasources that suits your special needs.

...

Besides, MsSql should be considered outdated long ago by MsSqlSpatial. Any reason you are not using this instead?


First: at this point in time, the database types (which I realize are different) are irrelevant. The objects contained in drx are "object { double }", which means it's a double value that's been boxed to an object type. So there is a potential loss of precision here, depending upon the length of values being returned. Also, this code will throw an exception so long as the underlying type is "object { double }".

These aren't my special needs. This is an obvious bug which potentially results in a loss of precision and an application crash. I also find it difficult to believe that converting from a double to a float and then back to a double aids in size or speed. The incoming data is: .NET double. The final result is: .NET double. What am I missing here?

Second: if MsSql.cs is considered outdated, then why not add Obsolete tags to the class with a suggestion to use MsSqlSpatial? The only reason I'm using MsSql.cs is because it was there when I needed it. I've never used SharpMap (or done any GIS programming prior to this point in time), so unless there's some sort of obvious warning tossed up, there isn't any reason for me to assume there's something better at my disposal.
Coordinator
Oct 31, 2007 at 10:20 PM
Hey kidjan -

Just to address your point on the MsSql provider as getting an ObsoleteAttribute added, I think it's a great idea. One of the things to note is that there isn't much maintenance being done on the v0.9 (v1.0) release at this time, since I have to put all my time into the v2.0 version. In v2.0, MsSql isn't even present, so adding an ObsoleteAttribute makes sense.
Coordinator
Oct 31, 2007 at 10:22 PM
This discussion has been copied to a work item. Click here to go to the work item and continue the discussion.