jdeolive / geodb

Spatial database bindings for Java.
MIT License
67 stars 25 forks source link

Extra functions added for compatibility with hibernate spatial #13

Closed jacques-finalist closed 12 years ago

jacques-finalist commented 12 years ago

Hi,

For our GIS projects we heavily rely on hibernate spatial. This works perfectly, but until recently we had to use a real database for testing. Last week I investigated the possibility of using GeoDB for this and was happily surprised at it's capabilities. I did however miss some spatial functions (for my current project for instance the extent function). Looking at the source of GeoDB it seemed easy to implement so this weekend I had a try. Could you review what I have done?

Greetings,

Jacques Bouman

jdeolive commented 12 years ago

Hi Jacques,

This looks really great, thanks for that. One thing though can we just add the extra functions to the GeoDB class directly, rather than a separate class. Other than that it looks great, will merge in and put out a new release. Thanks again.

jacques-finalist commented 12 years ago

Hi Justin,

I just moved the extra functions to the GeoDB class. Just to be curious, why do you prefer to keep all functions in the same class instead of creating a seperate class for each function (as no is done for Extent and Union for instance)?

Greetings,

Jacques

jdeolive commented 12 years ago

Hi Jacques,

Thanks for doing that. As for the organization of the functions I just figured they belonged with the other existing functions. Like for instance why put intersection in one class and union in another? I am open a reorganization of functions into specific classes but not sure exactly what that would be? Until then I prefer to keep things consolidated to a single class.

Also, to match conventions with the other functions in the GeoDB class, and the simple features for sql standard, i wplan to prefix all the function names with "ST_". Any objections?

-Justin

jacques-finalist commented 12 years ago

Hi Justin,

Just renamed the functions and moved them to a more logical place in de GeoDB class. Had some problems with the naming of the aggregate union functions. Apperently it is not allowed to have the same name for a function and an aggregate in H2. The aggregate now is named TT_Union, but feel free to give it a more logical name.

Giving each function each own class (and file) would have the benifit of giving more insight in the structure of the code. Navigating through the GeoDB class is now a little bit difficult and will become more difficult as you add more functions. Furthermore the chance of merge conflicts will rise if more people will start working on the code since almost all changes will probably be on the GeoDB class.

As for the naming of the functions. I initially tried to stick to the java conventions of giving methods Camel cased names starting with a lowercased letter.

Jacques

jdeolive commented 12 years ago

Hi Jacques,

Thanks again for this! I agree that it would be good to reorganize the functions as that main class is getting pretty crowded. As you can imagine the class started small but has ballooned since then :) I think a good organization would be to use the postgis function reference as a guide.

http://postgis.refractions.net/documentation/manual-1.3/ch06.html

And group the functions into appropriate classes. But let's leave that for a another issue (feel free to open it) and for now we can just stick then in GeoDB.java.

As for the naming convention of functions, i also think it makes sense to keep java naming conventions, but in the geodb.sql registering the function alias with the ST_prefix. That would fix the aggregate function naming issue. Perhaps we can open a separate issue for that as well.

Again thanks for this, I will get these changes merged in asap.

jdeolive commented 12 years ago

Hi Jacques,

About to merge this pull request, one question though. For aggregate function names that clash with regular function names i would like to come up with a naming convention. I am thinking either a. we drop the "ST_" prefix or we use a prefix/postfix like "Aggregate". So it would make for "Untion" or "ST_UntionAggregate" or "ST_AggregateUnion".

ANy preference?

jacques-finalist commented 12 years ago

I just looked up what at ST stands for (spatial type) and since these functions are also related to that i don't think that dropping the ST is a good choice. I would prefer to use your suggestion with the aggregate postfix, but add a underscore between the functionname and aggregate (ST_Union_Aggregate).