[postgis-devel] Re: Implementing support for PostGIS spatial types in JPOX

[Markus Schaber]

Hi, Thomas,

Thomas Marti (HSR) wrote:

> Well, it is already fixed...in the attached patches...

Yes, your code looks fine. Under the assumption that we don't like trickery

> 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."

Yes, you're right that it is not generally solvable with java.

>> 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

It won't work when mixing subclasses and unmodified base class.

It works if you only use subclasses, and all subclasses know of each
other. That's why I invented the equalsIntern() method.

The original Idea was that if "other" is a subclass of point, an
unmodified Point cross-calls the equalsintern method of the subclass
Point, and all subclasses know of each other. This is what Python does
internally for some operators (add and radd), and can be done in C++
with templates. But it was not fully coded.

And thinking about it, I'd rather prefer to make all classes final and
unmodifiable, I think this makes more sense and is the cleaner approach,
althoug we'll possibly brake compatibility with client code.

>> 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.

Strange, I unpacked the zip file in the src/orc/postgis directory and
diff -U showed nothing. But now it works. Weird.

>> 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.

I thought of _only_ a "cvs diff -u" in the project base directory, not
separate diffs for each class plus the full code. This should save the
most bandwith.

>> I just committed the equals() fix, as well as two getter methods for
>> Geometry, and am looking forward your reply to my other mails.

> 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.

Your code looks nice, and is easier to grasp than mine.

My ideas were far to complex.

I'll commit your code, and also add an appropriate copyright notice to
the files if you like.

> Some more ideas to "beautify" the code:
> * JtsGeometry.toString() should have a check for null

Yes, I'll add that.

> * ComposedGeom: use isEmpty() instead of ((subgeoms == null) ||
> (subgeoms.length == 0))

I'll also add final to isEmpty(), to allow the compiler to inline.

> 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.

AFAICS, all other PostGIS client class frameworks live in completely
independend projects (Python Geotypes come to my mind), so this
separation would make sense.

One problem is that CVS does not cleanly allow moves, but it can be
somehow simulated by working directly on the backend repository. It
should be as simple as "cp -a" the RCS subtree of the postgis/jdbc2
subdirectory into its own project directory (e. G. postgis_jdbc), and
then removing the jdbc2 directory in the PostGIS project via CVS commit.
This keeps all histories intact, but at the cost of duplicating it.

But having to maintain independend projects imposes a slightly higher
administrative burden for refractions, so it's their decision.

[... having all classes unmodifiable ...]

> 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.

Yes, and cleaning this up will both break existing code, and make some
operations less efficient.

However, as our classes are not really intended for big geometry
processing (use JTS for that), I think we can ignore that second problem.

> 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.

But one question is how far we go to enshure this constraint.

When looking at the JTS code, you'll see that they have some setters
either (e. G. setSRID()), and some getter methods and constructors don't
deep-copy the data.

The reason for this is to allow in-place modifications of intermediate
geometries while they are processed. They deal efficiency against
technical enforcement.

What would you propose for PostGIS JDBC? Should we simply announce "new
ComposedGeom(Geometry[])" make a deep copy of its arguments?

[ ... JTS-Support ... ]

> OK, it definitely sounds like a "worthy candidate" for our project... ;)

Feel free to submit bug fixes, ideas and regression tests if you want. :-)

>> 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...

Yes, you're right, the driver URL names are not really good. But when
changing that, I should keep the old ones working for compatibility reasons.

I also should add some more documentation...

Which names do you propose?


Btw, I just came across another optimization: Either make
geometry.getType() final, or abandon the type field altogether (saves 4
bytes per geometry), make getType() abstract, and put final
implementations into each subclass. If we go the road of breaking
compatibility, I think it may be worth the effort to go it completely.

Thanks,
Markus


-- 
Markus Schaber | Logical Tracking&Tracing International AG
Dipl. Inf.     | Software Development GIS

Fight against software patents in EU! www.ffii.org www.nosoftwarepatents.org