H2 trunk, fix Geometry

[Nicolas Fortin (OrbisGIS)]

Hi,

I made some fix on spatial h2 starting from the current trunk, it is here:

https://github.com/nicolas-f/h2database/tree/fix/isGeometry

The first fix, isGeometry function, a mix was done on Geometry object/class:

https://github.com/nicolas-f/h2database/commit/ac5fab9fffbf5cfd63f7e67e8ce4fa90da727dbc

Second fix, WKB export, SRID and Z was missing:

https://github.com/nicolas-f/h2database/commit/f8df369622defb0413aa15b8e78ee35b0d461ca8

Third fix, ValueGeometry.equals, GeometryCollection throws an error when equals is used, using a work around through binary comparison:

https://github.com/nicolas-f/h2database/commit/57d141acb4a97b960f704860994a283b33e69d40

Fourth fix, update OSGi bundle manifest in order to optionally import JTS and export some other package of h2 to be able to write a table engine in a remote OSGi bundle.

https://github.com/nicolas-f/h2database/commit/0042bf99cc677515cee6b91d4b8723e3330507bd

Thanks

-Nicolas Fortin
Atelier SIG
IRSTV FR CNRS 2488

[Noel Grandin]

On 2013-08-19 09:58, Nicolas Fortin (OrbisGIS) wrote:
>
> The first fix, isGeometry function, a mix was done on Geometry
> object/class:
>
> https://github.com/nicolas-f/h2database/commit/ac5fab9fffbf5cfd63f7e67e8ce4fa90da727dbc
>

This commit makes no sense at all.
- you have created a unnecessary recursive call in DataType#isGeometry
- you have broken the case where the geometry stuff is not on the classpath
- you have removed a test case from TestSpatial for no reason that I can
see.

> Second fix, WKB export, SRID and Z was missing:
>
> https://github.com/nicolas-f/h2database/commit/f8df369622defb0413aa15b8e78ee35b0d461ca8

This fix assumes that Geometry will always be 3D, which means that we
will no longer support 2D geometry, which does not seem like a good idea
to me.

>
> Third fix, ValueGeometry.equals, GeometryCollection throws an error
> when equals is used, using a work around through binary comparison:
>
> https://github.com/nicolas-f/h2database/commit/57d141acb4a97b960f704860994a283b33e69d40

This commit also makes no sense. Why is the Geometry class throwing an
IllegalArgumentException? We need more explanation here.

>
> Fourth fix, update OSGi bundle manifest in order to optionally import
> JTS and export some other package of h2 to be able to write a table
> engine in a remote OSGi bundle.
>
> https://github.com/nicolas-f/h2database/commit/0042bf99cc677515cee6b91d4b8723e3330507bd

I'm going to leave this for Thomas to comment on.
Historically we have been very loathe to expose more of H2's internals
because we have no intention of making all of this stuff part of our
"supported API".

[Nicolas Fortin (OrbisGIS)]

> The first fix, isGeometry function, a mix was done on Geometry
> object/class:
>
> https://github.com/nicolas-f/h2database/commit/ac5fab9fffbf5cfd63f7e67e8ce4fa90da727dbc
>
This commit makes no sense at all.
- you have created a unnecessary recursive call in DataType#isGeometry
- you have broken the case where the geometry stuff is not on the classpath
- you have removed a test case from TestSpatial for no reason that I can
see.

This is not recursive, it uses the other function that test class, not object.
the second function test if static geometry constant is null.
The random test case is covered by Thomas MVMap unit test in a cleaner way (without random, and way more faster)
 

> Second fix, WKB export, SRID and Z was missing:
>
> https://github.com/nicolas-f/h2database/commit/f8df369622defb0413aa15b8e78ee35b0d461ca8

This fix assumes that Geometry will always be 3D, which means that we
will no longer support 2D geometry, which does not seem like a good idea
to me.

You are right, we can replace the constructor by this:
new WKBWriter(geometry.getDimension(), geometry.getSRID() != 0)
 

>
> Third fix, ValueGeometry.equals, GeometryCollection throws an error
> when equals is used, using a work around through binary comparison:
>
> https://github.com/nicolas-f/h2database/commit/57d141acb4a97b960f704860994a283b33e69d40

This commit also makes no sense. Why is the Geometry class throwing an
IllegalArgumentException? We need more explanation here.

Geometry compute the intersection matrix (more robust to rounding errors), this cannot be done with GeometryCollection, then it throws an exception. Test all geometries inside the collection separatly may be another solution.
 

>
> Fourth fix, update OSGi bundle manifest in order to optionally import
> JTS and export some other package of h2 to be able to write a table
> engine in a remote OSGi bundle.
>
> https://github.com/nicolas-f/h2database/commit/0042bf99cc677515cee6b91d4b8723e3330507bd

I'm going to leave this for Thomas to comment on.
Historically we have been very loathe to expose more of H2's internals
because we have no intention of making all of this stuff part of our
"supported API".

There is the TableEngine in the api package, however it has dependency on other (hidden) packages.

[Noel Grandin]

On 2013-08-19 11:49, Nicolas Fortin (OrbisGIS) wrote:

> The first fix, isGeometry function, a mix was done on Geometry
> object/class:
>
> https://github.com/nicolas-f/h2database/commit/ac5fab9fffbf5cfd63f7e67e8ce4fa90da727dbc
>
This commit makes no sense at all.
- you have created a unnecessary recursive call in DataType#isGeometry
- you have broken the case where the geometry stuff is not on the classpath
- you have removed a test case from TestSpatial for no reason that I can
see.

This is not recursive, it uses the other function that test class, not object.
the second function test if static geometry constant is null.
The random test case is covered by Thomas MVMap unit test in a cleaner way (without random, and way more faster)

Ah, now that's more useful as a commit message.
OK, applied.

 

> Second fix, WKB export, SRID and Z was missing:
>
> https://github.com/nicolas-f/h2database/commit/f8df369622defb0413aa15b8e78ee35b0d461ca8

This fix assumes that Geometry will always be 3D, which means that we
will no longer support 2D geometry, which does not seem like a good idea
to me.

You are right, we can replace the constructor by this:
new WKBWriter(geometry.getDimension(), geometry.getSRID() != 0)

Applied.

 

>
> Third fix, ValueGeometry.equals, GeometryCollection throws an error
> when equals is used, using a work around through binary comparison:
>
> https://github.com/nicolas-f/h2database/commit/57d141acb4a97b960f704860994a283b33e69d40

This commit also makes no sense. Why is the Geometry class throwing an
IllegalArgumentException? We need more explanation here.

Geometry compute the intersection matrix (more robust to rounding errors), this cannot be done with GeometryCollection, then it throws an exception. Test all geometries inside the collection separatly may be another solution.

Hmm, this looks like it actually needs a fix to be applied in the JTS library.
I think you should liase with the JTS people and get them to fix their equals method.

 

>
> Fourth fix, update OSGi bundle manifest in order to optionally import
> JTS and export some other package of h2 to be able to write a table
> engine in a remote OSGi bundle.
>
> https://github.com/nicolas-f/h2database/commit/0042bf99cc677515cee6b91d4b8723e3330507bd

I'm going to leave this for Thomas to comment on.
Historically we have been very loathe to expose more of H2's internals
because we have no intention of making all of this stuff part of our
"supported API".

There is the TableEngine in the api package, however it has dependency on other (hidden) packages.

This patch exports a huge chunk of our internal stuff,  way more than the basic TableEngine API, so I'm going to leave it up to Thomas to make the call.

[Nicolas Fortin (OrbisGIS)]

I'm sorry, it's my mistake, Geometry.getDimension does not return 2 or 3. It is related to geometry type, not coordinates. The only way I found to test if a Geometry contain a Z coordinate somewhere is through the following function:

    /**
     * @param coordinates Coordinate array
     * @return True if the array is empty or contain only 2d coordinates.
     */
    private static boolean is2d(Coordinate[] coordinates) {
        if(coordinates==null) {
            return true;
        }
        for(Coordinate coordinate : coordinates) {
            if(!Double.isNaN(coordinate.z)) {
                return false;
            }
        }
        return true;
    }

Then the constructor become this

new WKBWriter(is2d(geometry.getCoordinates()) ? 2 : 3, geom3d.getSRID() != 0)

[Noel Grandin]

Thanks, updated trunk.

[Nicolas Fortin (OrbisGIS)]

I wrote a unit test:
https://github.com/nicolas-f/h2database/commit/ea22bd7532150a17a1e92b009a98bf389a7f3f7f#L1R533

If you take two geometries A "POLYGON ((67 13, 67 18, 59 18, 59 13, 67 13)) and B "POLYGON ((67 13 , 67 18 5, 59 18, 59 13, 67 13 ))"

The result of A.equal(B) is true.. Then the cache system of Value will keep only A and garbage B. The only way to fix this issue is to do a binary comparison in ValueGeometry.equals( )

[Noel Grandin]

No, that's a bug in the com.vividsolutions.jts.geom.Geometry#equals
method, which the JTS guys will need to fix.

Let us know when they have a fixed build available.

[Thomas Mueller]

Hi,

I made a few changes:

- Rename "getNoDimensions" to "getDimensionCount", because "getNoDimensions" is confusing for non-native English speaking people ("get no dimensions versus get yes dimensions" ?!)

- Rename the overloaded method "isGeometry" to "isGeometryClass" to avoid confusion. I know method overloading, but it shouldn't be used if the meaning of the method is different.

About com.vividsolutions.jts.geom.Geometry#equals, I'm sure we find a solution. I agree with Noel, that if the objects are not equal then the JTS suite shouldn't return true. But if it's not possible to fix the JTS suite within a reasonable time, then we might want to add a workaround. But document it as such.

Regards,
Thomas

--
You received this message because you are subscribed to the Google Groups "H2 Database" group.
To unsubscribe from this group and stop receiving emails from it, send an email to h2-database+unsubscribe@googlegroups.com.
To post to this group, send email to h2-da...@googlegroups.com.
Visit this group at http://groups.google.com/group/h2-database.
For more options, visit https://groups.google.com/groups/opt_out.

[Martin Davis]

Not a bug in JTS.  JTS Geometry.equals() computes *topological* equality, not exact representation equality.  The method is unfortunately named due to historical accident.

You may wish to use equalsExact - although this does not check for Z values. Other alternative is to write your own equalsExact3D.

[Martin Davis]

Sigh, yes, the JTS handling of Z values is a bit weak right now.  Hoping to improve this in redesign of the API - but it will break lots of things unfortunately.

In absence of better metadata about the coordinate dimension the method of checking for Z = NaN is about all that can be done right now, .  Be aware however that Geometry.getCoordinates() is a bit inefficient for complex geometries, since it has to create one big array to hold all Coordinate values, and then traverse the geometry copying the references.  Using a CoordinateFilter would be more efficient.

[Noel Grandin]

Thanks for commenting Martin.

If we did that, we'd have to use instanceof because GeometryCollection extends Geometry, which is icky.

I would really prefer that you fix this in JTS, even if it means creating yet another equals method.

[Noel Grandin]

Martin, it would be really nice if the WKBWriter
(a) had a constructor that simply takes an "includeSRID" parameter
(b) worked out the 2d/3d thing for itself.

[Nicolas Fortin (OrbisGIS)]

Hello Martin Davis,

As you advised I use the CoordinateSequenceFilter instead of iterating over vertices:
https://github.com/nicolas-f/h2database/commit/d168e04e061d470aebbc8749ce9663e97a7da18f

About the OSGi manifest. If you don't want to expose more package in OSGi, how to use org.h2.api.TableEngine as it imports org.h2.table.TableBase ?

Thanks for support

-Nicolas Fortin
Atelier SIG
IRSTV FR CNRS 2488

[Thomas Mueller]

Hi,

> About the OSGi manifest. If you don't want to expose more package in OSGi, how to use org.h2.api.TableEngine as it imports org.h2.table.TableBase ?

Yes, that's a good question :-) And the method passes a CreateTableData, which is in another package. There is one small change I will make, but this will not solve the problem: TableEngine should return a Table, not a TableBase. But to solve the dependency problem, yes I guess the easiest solution is to change the OSGi manifest. I don't like to do that, but currently don't see an alternative.

Regards,

Thomas

--

You received this message because you are subscribed to the Google Groups "H2 Database" group.

To unsubscribe from this group and stop receiving emails from it, send an email to h2-database...@googlegroups.com.

[Christoph Läubrich]

You can extract an interface out of

CreateTableData

with getter/setter method place it in the API package and let the original CreateTableData implement that interface.

In fact to prevent this kind of issue its alwaysa a good idea in OSGi to have one api bundle that does only contain interfaces and Exception classes and keep the implementation in a seperate bundle that imports the interface classes.
That way you can't accidently use internal classes in the API and the api bundle is the only one that is imported by users as well as implementation and there is no direct dependency on the implementor of that API.

[Nicolas Fortin (OrbisGIS)]

I will use a (dirty) workaround. I will create a Fragment-Bundle (a bundle that share the same class loader as H2) then do not import in manifest (org.h2.command.ddl,org.h2.engine,org.h2.message,org.h2.index,org.h2.table,org.h2.result)

[Nicolas Fortin (OrbisGIS)]

Some things are still missing:

• Equals method of Geometry is not the one expected by ValueGeometry, we should do a wkb binary comparison as it is the only way to avoid false duplicate. (The cache return another geometry with other srid or z)

• JTS is not imported in the H2 manifest

[Noel Grandin]

On 2013-08-26 10:54, Nicolas Fortin (OrbisGIS) wrote:

Some things are still missing:

• Equals method of Geometry is not the one expected by ValueGeometry, we should do a wkb binary comparison as it is the only way to avoid false duplicate. (The cache return another geometry with other srid or z)

I maintain that this is a bug in JTS that needs to be fixed in JTS.

• JTS is not imported in the H2 manifest

Sorry, I seem to have missed that - do you have a patch somewhere?

[Nicolas Fortin (OrbisGIS)]

JTS import is here:
https://github.com/nicolas-f/h2database/commit/0042bf99cc677515cee6b91d4b8723e3330507bd

About GeometryValue.equals they answered that this is the expected result http://sourceforge.net/p/jts-topo-suite/bugs/40/ .

-Nicolas Fortin
Atelier SIG
IRSTV FR CNRS 2488

[Noel Grandin]

On 2013-08-26 11:51, Nicolas Fortin (OrbisGIS) wrote:

JTS import is here:
https://github.com/nicolas-f/h2database/commit/0042bf99cc677515cee6b91d4b8723e3330507bd

I've already said that patch is not acceptable.

About GeometryValue.equals they answered that this is the expected result http://sourceforge.net/p/jts-topo-suite/bugs/40/ .

Sigh.

[Nicolas Fortin (OrbisGIS)]

I've already said that patch is not acceptable.

Just take the lines 33,34