GeoTools

Update query api to support joins

Details

  • Type: Improvement Improvement
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: None
  • Fix Version/s: 8.0-M3
  • Component/s: api, data
  • Labels:
    None

Description

Adding ability to do joins in a data store query.

Issue Links

is depended upon by

New Feature - A new feature of the product, which has yet to be developed. GEOS-4634 WFS 2.0 - Joins

  • Major - Major loss of function.
  • Closed - The issue is considered finished, the resolution is correct. Issues which are not closed can be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Jody Garnett added a comment -
Justin did you want to do one proposal to cover all the WFS 2.0 filter changes? Or keep handling things one at a time ... is there much more to come after Joins?
Show
Jody Garnett added a comment - Justin did you want to do one proposal to cover all the WFS 2.0 filter changes? Or keep handling things one at a time ... is there much more to come after Joins?
Hide
Permalink
Justin Deoliveira added a comment -
Nope, joins are pretty much the last thing on my plate. And yes, I think it makes sense to separate them out.
Show
Justin Deoliveira added a comment - Nope, joins are pretty much the last thing on my plate. And yes, I think it makes sense to separate them out.
Hide
Permalink
Justin Deoliveira added a comment -
Updated patch that contains the method rename for app-schema. Ben can your review? Thanks.
Show
Justin Deoliveira added a comment - Updated patch that contains the method rename for app-schema. Ben can your review? Thanks.
Hide
Permalink
Ben Caradoc-Davies added a comment -
+1 for the app-schema part, Justin.
Show
Ben Caradoc-Davies added a comment - +1 for the app-schema part, Justin.
Hide
Permalink
Justin Deoliveira added a comment -
Adding Andrea and Christian as watchers since much of this work is in jdbc land.
Show
Justin Deoliveira added a comment - Adding Andrea and Christian as watchers since much of this work is in jdbc land.
Hide
Permalink
Christian Mueller added a comment -
The last patch shows errors when applying to trunk. Can somebody check before I start investigations ?
Show
Christian Mueller added a comment - The last patch shows errors when applying to trunk. Can somebody check before I start investigations ?
Hide
Permalink
Justin Deoliveira added a comment -
Hi Christian, are you talking about compilation errors? Or test failures. I just applied the patch to the trunk and did a full build with no compilation errors. However there is an error I get in the app-schema module. Not sure if it is related to the patch or just one of those local failures for me. I wil have to ask Ben et al about that.
Show
Justin Deoliveira added a comment - Hi Christian, are you talking about compilation errors? Or test failures. I just applied the patch to the trunk and did a full build with no compilation errors. However there is an error I get in the app-schema module. Not sure if it is related to the patch or just one of those local failures for me. I wil have to ask Ben et al about that.
Hide
Permalink
Christian Mueller added a comment -
No, the "patch -p1 < GEOT-3657.patch" command shows errors for 4 deleted files.

JDBCForeignkeyTest.java as an example.

I removed the files manually. So far, so good.
Show
Christian Mueller added a comment - No, the "patch -p1 < GEOT-3657 .patch" command shows errors for 4 deleted files. JDBCForeignkeyTest.java as an example. I removed the files manually. So far, so good.
Hide
Permalink
Justin Deoliveira added a comment -
Still confused what "error" means. Is patch complaining? Indeed some files were removed... all the jdbc association stuff that didn't get used and simply polluted the feature reader implementation.
Show
Justin Deoliveira added a comment - Still confused what "error" means. Is patch complaining? Indeed some files were removed... all the jdbc association stuff that didn't get used and simply polluted the feature reader implementation.
Hide
Permalink
Christian Mueller added a comment -
I tested on a second box and got the same errors

patching file modules/library/jdbc/src/test/java/org/geotools/jdbc/JDBCForeignKeyTest.java
Reversed (or previously applied) patch detected! Assume -R? [n]

patching file modules/library/jdbc/src/test/java/org/geotools/jdbc/JDBCGeometryAssociationTestSupport.java
Reversed (or previously applied) patch detected! Assume -R? [n]

patching file modules/plugin/jdbc/jdbc-h2/src/test/java/org/geotools/data/h2/H2ForeignKeyTest.java
Reversed (or previously applied) patch detected! Assume -R? [n]

patching file modules/plugin/jdbc/jdbc-h2/src/test/java/org/geotools/data/h2/H2GeometryAssociationTest.java
Reversed (or previously applied) patch detected! Assume -R? [n]

Deleted the files manually.
Show
Christian Mueller added a comment - I tested on a second box and got the same errors patching file modules/library/jdbc/src/test/java/org/geotools/jdbc/JDBCForeignKeyTest.java Reversed (or previously applied) patch detected! Assume -R? [n] patching file modules/library/jdbc/src/test/java/org/geotools/jdbc/JDBCGeometryAssociationTestSupport.java Reversed (or previously applied) patch detected! Assume -R? [n] patching file modules/plugin/jdbc/jdbc-h2/src/test/java/org/geotools/data/h2/H2ForeignKeyTest.java Reversed (or previously applied) patch detected! Assume -R? [n] patching file modules/plugin/jdbc/jdbc-h2/src/test/java/org/geotools/data/h2/H2GeometryAssociationTest.java Reversed (or previously applied) patch detected! Assume -R? [n] Deleted the files manually.
Hide
Permalink
Andrea Aime added a comment -
Hi Justin,
took some time to review the patch today. In my case it applies fine, I have no conflicts or issues with deleted files.

Little things:
* most new classes miss copyright headers
* most modified classes have unused imports

Besides this, wow, a lot of work, took quite some time to wrap my head around it. From what I can see the code is good, thought I have to say I'm far away from fully understanding all the implementation details.

Looking at the tests I've noticed the following:
* Oracle has a test failing, testSpatialJoin fails with:
  java.lang.RuntimeException: Subclasses must implement this method in order to handle geometries
at org.geotools.data.jdbc.FilterToSQL.visitBinarySpatialOperator(FilterToSQL.java:943)
at org.geotools.data.jdbc.FilterToSQL.visitBinarySpatialOperator(FilterToSQL.java:920)
at org.geotools.data.jdbc.FilterToSQL.visit(FilterToSQL.java:846)
* I see the join tests are implemented for H2, Postgis and Oracle, while the temporal tests are available
  only for H2 and PostGIS. I guess the respective maintainers of the other databases will have to implement those later,
  along with actual support for temporal filters (which is declared only in the filter caps of H2 and PostGIS)
* exposing primary keys might be messing up with joining, I believe it would be good to have a joining test with
  the exposed pks
* wonderins, what would happen if a Join property set is empty, that is, Query.NO_PROPERTIES?

From the "it would be nice department":
* I just noticed remove/update are out of the join game, pity, it will prevent queries like "remove all post officies in the Boulder county"
  where the county comes from another layer
* somewhere in the API it would be nice to have a flattener that turns back the result of a join in a flat feature type with no nested
  features
Show
Andrea Aime added a comment - Hi Justin, took some time to review the patch today. In my case it applies fine, I have no conflicts or issues with deleted files. Little things: * most new classes miss copyright headers * most modified classes have unused imports Besides this, wow, a lot of work, took quite some time to wrap my head around it. From what I can see the code is good, thought I have to say I'm far away from fully understanding all the implementation details. Looking at the tests I've noticed the following: * Oracle has a test failing, testSpatialJoin fails with:   java.lang.RuntimeException: Subclasses must implement this method in order to handle geometries at org.geotools.data.jdbc.FilterToSQL.visitBinarySpatialOperator(FilterToSQL.java:943) at org.geotools.data.jdbc.FilterToSQL.visitBinarySpatialOperator(FilterToSQL.java:920) at org.geotools.data.jdbc.FilterToSQL.visit(FilterToSQL.java:846) * I see the join tests are implemented for H2, Postgis and Oracle, while the temporal tests are available   only for H2 and PostGIS. I guess the respective maintainers of the other databases will have to implement those later,   along with actual support for temporal filters (which is declared only in the filter caps of H2 and PostGIS) * exposing primary keys might be messing up with joining, I believe it would be good to have a joining test with   the exposed pks * wonderins, what would happen if a Join property set is empty, that is, Query.NO_PROPERTIES? From the "it would be nice department": * I just noticed remove/update are out of the join game, pity, it will prevent queries like "remove all post officies in the Boulder county"   where the county comes from another layer * somewhere in the API it would be nice to have a flattener that turns back the result of a join in a flat feature type with no nested   features
Hide
Permalink
Christian Mueller added a comment -
DB2 temporal and join tests are almost ready. I will attach a zip file since the patch file contains already patches for DB2 classes.
Show
Christian Mueller added a comment - DB2 temporal and join tests are almost ready. I will attach a zip file since the patch file contains already patches for DB2 classes.
Hide
Permalink
Christian Mueller added a comment -
Zip files containing modified db2 classes. I will commit this changes after the patch is applied.

All tests passed (total 269 tests)
Show
Christian Mueller added a comment - Zip files containing modified db2 classes. I will commit this changes after the patch is applied. All tests passed (total 269 tests)
Hide
Permalink
Justin Deoliveira added a comment -
Thanks for the review and for getting the tests passing on db2. I just posted a new patch that addresses the following:

* spatial and temporal joins implemented for oracle, tests passing
* added flag to run all join tests with/without primary keys exposed
* added a test for QUery.NO_PROPERTIES (thanks for that Andrea, there was indeed a bug there)
* cleaned up imports and added missing copyright headers

I agree that moving forward joins with remove/updates would be great. And the api to control the structure of the feature. However for now I don't have the mandate to start implementing those. But I don't see any blockers there.

So, if the new patch sounds good and it is acceptable to leave the "nice to haves" as future improvements I would like to commit the patch tomorrow.

Thanks again for the review guys, i know this was no small patch, the work is much appreciated.
Show
Justin Deoliveira added a comment - Thanks for the review and for getting the tests passing on db2. I just posted a new patch that addresses the following: * spatial and temporal joins implemented for oracle, tests passing * added flag to run all join tests with/without primary keys exposed * added a test for QUery.NO_PROPERTIES (thanks for that Andrea, there was indeed a bug there) * cleaned up imports and added missing copyright headers I agree that moving forward joins with remove/updates would be great. And the api to control the structure of the feature. However for now I don't have the mandate to start implementing those. But I don't see any blockers there. So, if the new patch sounds good and it is acceptable to leave the "nice to haves" as future improvements I would like to commit the patch tomorrow. Thanks again for the review guys, i know this was no small patch, the work is much appreciated.
Hide
Permalink
Justin Deoliveira added a comment -
New (and hopefully final) patch with temporal and join tests/support for mysql and sql server
Show
Justin Deoliveira added a comment - New (and hopefully final) patch with temporal and join tests/support for mysql and sql server
Hide
Permalink
Justin Deoliveira added a comment -
Ok, patch committed to trunk. Christian if you can apply the db2 stuff and then resolve the issue taht would be great. Thanks.
Show
Justin Deoliveira added a comment - Ok, patch committed to trunk. Christian if you can apply the db2 stuff and then resolve the issue taht would be great. Thanks.
Hide
Permalink
Christian Mueller added a comment -
Committed the db2 stuff, all tests passed.
Show
Christian Mueller added a comment - Committed the db2 stuff, all tests passed.

People

  • Assignee:
    Justin Deoliveira
    Reporter:
    Justin Deoliveira
Vote (0)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: