[postgis-devel] Re: Implementing support for PostGIS spatial types
in JPOX
Thomas Marti (HSR)
tmarti at hsr.ch
Tue May 9 08:00:27 PDT 2006
Hello everyone
Let's continue this discussion...
Markus Schaber wrote:
>> equals() does not conform to the general contract (throws an
>> exception when null is given)
>
> This is a bug, I'll look into fixing it.
Well, it is already fixed...in the attached patches...
>> equals() uses 'getClass()' instead of 'instanceof'
>
> The reason for this is that I intentionally wanted to avoid having the
> problem of comparing different subclasses.
>
> Say we have an Object a of MyPoint, and an Object b of YourPoint, both
> extending Point.
>
> Now the code has to see that both of them are a subclass of point, and
> then compare them, having no knowledge about the exact semantics of
> MyPoint and YourPoint.
Well, to be honest with you. You're trying to solve a problem that
cannot be solved! To quote Joshua Bloch's excellent book 'Effective
Java' (Chapter 3, Item 7):
"There is simply no way to extend an instantiable class and add an
aspect while preserving the equals contract."
In other words: Every class that extends Point and adds fields, whos
values are relevant for equality will automatically break the equals()
contract!
With that in mind, the current implementation isn't really helping
anyone. Not the extending subclass that adds aspects and not the
extending subclass which doens't add aspects (like ours). The problem
is, you can't try to let a superclass "think" for it's subclasses. But
you can provide suitable methods, like equals() implemented with
instanceof, that are open for extension. When a subclass has other
needs, it has to override the method, but that is none of the
superclasses business. We really hope you reconsider this.
> For your case, you should override the equals method in your subclass
> with appropriate semantics, and then possibly the equalsIntern method.
This doesn't solve the problem, because we can't ensure symmetry.
Example:
Point p1 = new Point( 1, 1 );
PointWrapper p = new PointWrapper( 1, 1 ); // equals() overridden
p2.equals( p1 ); // true
p1.equals( p2 ); // false, because of getClass()!
> It seems that you had attached the original, unmodified versions of the
> classes.
>
Uhm, actually we didn't. If you look closely you'll see the changes we
made in equals(). We also "killed off" equalsintern(), because we think
it isn't needed.
> Btw, I prefer a "diff -u" attached as text/plain instead of .zip, as
> it clearly shows the differences and can be looked at directly from
> the MUA.
Sorry for that, wanted to save some space. I attached the plain diffs
adn all java classes again in this mail.
[snip - taken from the third mail]
> I just committed the equals() fix, as well as two getter methods for
> Geometry, and am looking forward your reply to my other mails.
>
> Btw, as the JDBC code is relatively independent from the rest of
> PostGIS, we should discuss separating releases, e. G. by moving the
> JDBC part into its own CVS directory, just as the PostgreSQL people
> moved all non-libpq client libraries into their own projects.
>
> This allows for different minor releases of PostGIS and PostGIS-JDBC.
[snip]
Well, you were a bit too fast for us... ;) Because of the scenario
described above we still favour "our version". We also think the code is
bit more compact, while retaining the same functionality. Also, our
approach follows the recommended protocol for implementing equals() as
described in "Effective Java" and elsewhere.
Some more ideas to "beautify" the code:
* JtsGeometry.toString() should have a check for null
* ComposedGeom: use isEmpty() instead of ((subgeoms == null) ||
(subgeoms.length == 0))
About dividing PostGIS-JDBC and the geometry classes: This sounds like a
very good idea to us, but you're the experts who have to decide this.
>> Is there any way to make them private in future releases?
>
> I had thought about doing this, but it will possibly break compatibility
> with existing software.
>
> We should take two steps for this:
>
> - enshure that all attributes have proper accessor methods, could be in
> a minor release
>
> - change all non-final member variables to private (or at least
> protected), this should be a 2.x or at least 1.x release, not in
1.1.x IMHO.
[snip - taken from the second mail]
> There's another discussion topic here:
>
> At least for JTS, their general design approach is to treat all geometry
> objects as unmodifiable, and instead of modifying a geometry in place,
> operations create new geometries. I know that their code does not 100%
> reflect his, but it is what they advise.
>
> Having a similar model for PostGIS would mean to make all public fields
> final, or to only provide getter methods and no setter methods. How
> would this interfere with your plans?
[snip]
We're very glad, that you brought this up.
Right now the classes are in a sort of mixed state: You can change any
field in Point and Geometry (because they're public and have setters),
but not in the other classes (because subgeoms is protected and has no
setters). But then again you can get a single Point of a Polygon,
LineString or whatever and change that. So the current implementation
isn't very consistent.
From a technical point of view, either way (mutable or immutable) is
fine with us, but it has to be consistent! From a design point of view,
we would strongly favour an immutable implementation across all Geometry
classes, because we feel this makes the most sense and also is
consistent with JTS.
>> 3.) How far developed is JTS-Support in the PostGIS-JDBC-Driver? The
>> todo-file is rather sparse and says only 'Finish JTS support'. Can
>> anyone state how complete the implementation approximately is?
>> 20% or 80%?
>
> It is fully working, and in productional use at our site.
>
> However, the regression tests are not extensive enough, the build
> process is suboptimal, and I still have to rework the whole
> GeometryFactory / SRID thing to avoid the strange geometry factory
> cache, now as JTS does not deprecate the SRID fields any more.
>
> And there are bugs in code pathes I did not use in my own projects (as
> the one you found below), due to the lack of user feedback and
> regression tests.
OK, it definitely sounds like a "worthy candidate" for our project... ;)
>> 4.) What is the difference between JtsWrapper and JtsGisWrapper? The
>> only things we found, when comparing the source code was:
>> * the POSTGIS_PROTOCOL differs: "jdbc:postgresql_JTS:" vs.
>> "jdbc:postgres_jts:"
>> * JtsGisWrapper adds 'geometry', 'box2d' and 'box3d' as datatypes.
>> JtsWrapper adds only 'geometry'. But where is the difference in the
>> implementation?
>
> There is no other difference, the JtsWrapper only registers a handler
> to map the JTS classes on the PostGIS geometry PostgreSQL datatype,
> whereas the JtsGisWrapper additionally uses the plain PostGIS
> box2d/box3d classes to handle bounding boxes.
Hmm, then maybe setting a more descriptive name, rather than just use
'jts' in upper and lower case, could help prevent confusion in the future...
>> 5.) We also think that we've found a bug in
>> org.postgis.jts.JtsGeometry#equals():
>
>> return ((JtsGeometry) obj).getValue().equals(geom);
>
> Yes, that's a bug, AFAICS it should have read
>
> return ((JtsGeometry) obj).getGeometry().equals(geom);
>
> But that still does not fix the "null" issue, I'll overhaul it and
> commit the fix as soon as CVS access works again.
Actually it does! The instanceof-operator returns false, if the argument
is null. So, one thing less, that you have to worry about... ;)
I made a patch for this as well, it is attached.
> Thanks a lot,
> Markus
Thank YOU... ;)
-------------- next part --------------
Index: org/postgis/Point.java
===================================================================
RCS file: /home/cvs/postgis/postgis/jdbc2/src/org/postgis/Point.java,v
retrieving revision 1.11
diff -u -r1.11 Point.java
--- org/postgis/Point.java 16 Nov 2005 13:04:30 -0000 1.11
+++ org/postgis/Point.java 9 May 2006 14:12:34 -0000
@@ -45,14 +45,14 @@
return (int) (v ^ (v >>> 32));
}
- protected boolean equalsintern(Geometry otherg) {
- Point other = (Point) otherg;
- return equals(other);
- }
-
- public final boolean equals(Point other) {
- boolean xequals = x == other.x;
- boolean yequals = y == other.y;
+ public boolean equals(Object obj) {
+ if (!(obj instanceof Point)) return false;
+ if (!super.equals(obj)) return false;
+
+ Point other = (Point)obj;
+
+ boolean xequals = (x == other.x);
+ boolean yequals = (y == other.y);
boolean zequals = ((dimension == 2) || (z == other.z));
boolean mequals = ((haveMeasure == false) || (m == other.m));
boolean result = xequals && yequals && zequals && mequals;
-------------- next part --------------
Index: org/postgis/ComposedGeom.java
===================================================================
RCS file: /home/cvs/postgis/postgis/jdbc2/src/org/postgis/ComposedGeom.java,v
retrieving revision 1.7
diff -u -r1.7 ComposedGeom.java
--- org/postgis/ComposedGeom.java 28 Jul 2005 12:23:16 -0000 1.7
+++ org/postgis/ComposedGeom.java 9 May 2006 14:31:45 -0000
@@ -26,11 +26,12 @@
package org.postgis;
-import org.postgresql.util.PGtokenizer;
-
import java.sql.SQLException;
+import java.util.Arrays;
import java.util.Iterator;
+import org.postgresql.util.PGtokenizer;
+
/**
* ComposedGeom - Abstract base class for all Geometries that are composed out
* of other Geometries.
@@ -126,27 +127,13 @@
*/
protected abstract Geometry[] createSubGeomArray(int size);
- protected boolean equalsintern(Geometry other) {
- // Can be assumed to be the same subclass of Geometry, so it must be a
- // ComposedGeom, too.
+ public boolean equals(Object other) {
+ if (!(other instanceof ComposedGeom)) return false;
+ if (!super.equals(other)) return false;
+
ComposedGeom cother = (ComposedGeom) other;
- if (cother.subgeoms == null && subgeoms == null) {
- return true;
- } else if (cother.subgeoms == null || subgeoms == null) {
- return false;
- } else if (cother.subgeoms.length != subgeoms.length) {
- return false;
- } else if (subgeoms.length == 0) {
- return true;
- } else {
- for (int i = 0; i < subgeoms.length; i++) {
- if (!cother.subgeoms[i].equalsintern(this.subgeoms[i])) {
- return false;
- }
- }
- }
- return true;
+ return Arrays.equals(this.subgeoms, cother.subgeoms);
}
public int numPoints() {
-------------- next part --------------
Index: org/postgis/Geometry.java
===================================================================
RCS file: /home/cvs/postgis/postgis/jdbc2/src/org/postgis/Geometry.java,v
retrieving revision 1.8
diff -u -r1.8 Geometry.java
--- org/postgis/Geometry.java 9 May 2006 13:06:55 -0000 1.8
+++ org/postgis/Geometry.java 9 May 2006 14:00:50 -0000
@@ -136,35 +136,19 @@
/**
* java.lang.Object equals implementation
*/
- public boolean equals(Object other) {
- return (other != null) && (other instanceof Geometry) && equals((Geometry) other);
- }
+ public boolean equals(Object obj) {
+ if (obj == this) return true;
+ if (!(obj instanceof Geometry)) return false;
+
+ Geometry other = (Geometry)obj;
+
+ boolean firstline = (this.dimension == other.dimension) && (this.type == other.type);
+ boolean secondline = (this.srid == other.srid) && (this.haveMeasure == other.haveMeasure);
- /**
- * geometry specific equals implementation - only defined for non-null
- * values
- */
- public boolean equals(Geometry other) {
- boolean firstline = (other != null) && (this.dimension == other.dimension)
- && (this.type == other.type);
- boolean sridequals = (this.srid == other.srid);
- boolean measEquals = (this.haveMeasure == other.haveMeasure);
- boolean secondline = sridequals && measEquals;
- boolean classequals = other.getClass().equals(this.getClass());
- boolean equalsintern = this.equalsintern(other);
- boolean result = firstline && secondline && classequals && equalsintern;
- return result;
+ return firstline && secondline;
}
/**
- * Whether test coordinates for geometry - subclass specific code
- *
- * Implementors can assume that dimensin, type, srid and haveMeasure are
- * equal, other != null and other is the same subclass.
- */
- protected abstract boolean equalsintern(Geometry other);
-
- /**
* Return the number of Points of the geometry
*/
public abstract int numPoints();
-------------- next part --------------
Index: org/postgis/jts/JtsGeometry.java
===================================================================
RCS file: /home/cvs/postgis/postgis/jdbc2/jtssrc/org/postgis/jts/JtsGeometry.java,v
retrieving revision 1.11
diff -u -r1.11 JtsGeometry.java
--- org/postgis/jts/JtsGeometry.java 9 May 2006 13:06:56 -0000 1.11
+++ org/postgis/jts/JtsGeometry.java 9 May 2006 13:39:11 -0000
@@ -140,14 +140,14 @@
}
public boolean equals(Object obj) {
- if ((obj != null) && (obj instanceof JtsGeometry)) {
- Geometry other = ((JtsGeometry) obj).geom;
- if (this.geom == other) { // handles identity as well as both ==null
- return true;
- } else if (this.geom != null && other != null) {
- return other.equals(this.geom);
- }
+ if (obj == this) return true;
+ if (!(obj instanceof JtsGeometry)) return false;
+
+ Geometry other = ((JtsGeometry) obj).geom;
+ if (other == null) {
+ return (this.geom == null);
+ } else {
+ return other.equals(this.geom);
}
- return false;
}
}