Read SQL Servers binary format directly

[bakk]

Read SQL Server binary directly into JTS Geometries, instead of going
via WKB. While geotools will use a little longer to decode (about 50% more
compared to WKB), SQL Server won't have to encode at all, resulting in a performance
improvement.

A simple benchmark showed a performance increase around 50% on a
layer with many small geometries (a grid), while another layer only showed
7% performance increase.

The binary reader only support v1 of the specification. V2 adds support for CircularString, CompoundCurve, CurvePolygon, FullGlobe (http://go.microsoft.com/fwlink/?LinkId=226407), and is not supported by today's WKB implementation either (as far as I can see).

It would be nice if the added classes (for parsing the sql server binary) could be licenced freely (eg. apache/bsd), but if you would like it to be LGPL like the rest, that's OK too.

It's important to review this thoroughly so that everything that today's implementation supports still works :)

[aaime]

Everything that becomes part of GeoTools has to be licenced under LGPL. I'll have a look at the patches as spare time allows (or maybe Justin wants to?). Wondering, are V1 and V2 specs compatible when it comes to the same geometry type? Like, Point, Line and Polygon are encoded the same way?
Given this is new code, I'm wondering if having a configurable choice between WKB and native binary would be better

[bakk]

About the license: that's ok.
I forgot to include a link to the actual spec: http://msdn.microsoft.com/en-us/library/ee320529(v=sql.105).aspx
The differences between V1 and V2 are:

• new geometry types, as mentioned
• a new flag to indicate that a geography is larger than a hemisphere
• a segments-structure (used for compound curves)
• the definition of "figure attribute" has been changed

To support points, lines, polygons in V2, you would at least have to support the new definition of "figure attribute". Otherwise the formats are the same. However, I downloaded the latest SQL Server 2012 release, and when I created a new geometry, I got version 1. I've also tested with SQL Server 2008 R2. So the fact that I wasn't able to produce test-data with SQL Server stopped me from implementing partial support for V2.

If you could point me in the right direction of how to make the choice configurable, I can implement it. How would the user specify it - in the GUI?

[aaime]

Just add a new configuration parameter in the factory, that gets eventually set in the dialect, like the one for supporting native paging. The GUI in GeoServer is automatically generated from the parameter list (you'll have to also add it to the list of params advertised by the factory, and its jndi equivalent).

[bakk]

I've added a new configuration parameter. It defaults to using WKB. If the new code proves to be stable, I suggest switching default settings later.

[jdeolive]

We try to stick to the convention of keeping package names lowercase.

[jdeolive]

First off great work! This is something i have wanted to see for a while since as you note it can have a huge performance impact.

I think the patch looks great for the most part, except for the optional configuration parameter, which as implemented will still change the default for people upgrading. I think instead of explicitly having to enable wkb serialization we should have to explicitly enable native serializastion. So i would change the parameter to "NATIVE_SERIALIZATION" and have it by default set to false.

As you say if it shows to be stable we can make it the default in a future version.

[bakk]

You are correct - changing default for people upgrading was unintended. I've corrected it, and I also took the liberty to move the parameter up by one position in the list of advertised parameters. I just thought it looked better in GeoServer. The casing of the parameters (as they appear in GeoServer) is inconsistent, so I'm not sure if there is a correct way.

Another thing: I believe all the tests now run with the default setting (WKB), so the native implementation is not tested (other than the unit tests which test serialization directly). Not sure if this is a problem.

[jdeolive]

Great, thanks for changing that. And yeah, the casing is inconsistent so i wouldn't worry about that.

Indeed it would be nice to have at least one of the tests run with the native serialization, ideally one of the tests that involves reading geometries, like SQLServerFeatureSourceTest. It should just be a matter of extending the existing class and overriding the setupDataStore method to set the native serialization. But I will leave that up to you, i don't see it as a blocker for this change since it already has a good test that exercise the changes.

So as far as i am concerned the patch is good but i'll give Andrea a chance to weigh in as well before merge.

[aaime]

Thanks a lot, looking good. I've manually merged it and added a few of changes:

• there was a package named Reader instead of reader, as a result the code would not even compile on a case sensitive file system like Linux has, fixed it
• made it clear that the serialization format is for the geometries ("use native geometry serialization format")
• tested it with several geometry types with success, observed the speedup by the naked eye on large enough data sets
• created a ticket in jira (http://jira.codehaus.org/browse/GEOT-4442), squashed all commits to one and reworded the commit message to refer to the jira ticket

The full result is here:
9d7f87d

Given that the behavior is optional and I've found no issues in the code or tests, I've also backported the patch to the stable series (9.x).

[bakk]

That's great! Thank you both for reviewing it so quickly!