You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@jena.apache.org by GitBox <gi...@apache.org> on 2022/02/24 10:56:00 UTC
[GitHub] [jena] LorenzBuehmann opened a new pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
LorenzBuehmann opened a new pull request #1204:
URL: https://github.com/apache/jena/pull/1204
Fixes a bug where the precomputed geospatial index isn't loaded from file and thus not being registered to the dataset context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] Aklakan edited a comment on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
Aklakan edited a comment on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1062209248
Just a quick comment for robustness: You probably want to write the index to a temp file first and only if that completes successfully then copy it to the final destination - otherwise if you hit ctrl+c during the build a restart would try to load the partially written file.
(If you are doing it like this anyway then ignore my remark :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] LorenzBuehmann commented on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
LorenzBuehmann commented on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1055153216
well, technically just providing an non-empty precomputed index file + querying would trigger the error.
But yes, reproduction via assembler would basically take two runs, the first would generate and save the spatial index, in the second run it would simply load the index and register it to the dataset context such that when querying the geospatial extension can make use of it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] galbiston commented on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
galbiston commented on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1057214711
Is an additional function needed?
`GeoSPARQLConfig.setupSpatialIndex(dataset, spatialIndexPath.toFile());` ultimately goes to `SpatialIndex.buildSpatialIndex(Dataset dataset, String srsURI, File spatialIndexFile)`.
`buildSpatialIndex` loads the file and if it results in an empty SpatialIndex will then build a new one based on the `dataset`.
```
public static SpatialIndex buildSpatialIndex(Dataset dataset, String srsURI, File spatialIndexFile) throws SpatialIndexException {
SpatialIndex spatialIndex = load(spatialIndexFile);
if (spatialIndex.isEmpty()) {
Collection<SpatialIndexItem> spatialIndexItems = findSpatialIndexItems(dataset, srsURI);
save(spatialIndexFile, spatialIndexItems, srsURI);
spatialIndex = new SpatialIndex(spatialIndexItems, srsURI);
spatialIndex.build();
}
setSpatialIndex(dataset, spatialIndex);
return spatialIndex;
}
```
Is the fix not to remove the `if ( ! Files.exists(spatialIndexPath) || Files.size(spatialIndexPath) == 0 )` check?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1077562630
As a style consideration, personally, I would keep `setupPrecomputedSpatialIndex` (clear name for an operation, somewhere to put javadoc).
Shall we merge as-is and iterate as people feel there are improvements to make?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] LorenzBuehmann edited a comment on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
LorenzBuehmann edited a comment on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1057732717
@galbiston
I'm not sure, but `setupSpatialIndex` does always scan for all SRS URIs first, I don't think this is efficient as it will increase the startup time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] Aklakan edited a comment on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
Aklakan edited a comment on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1062209248
Just a quick comment for robustness: You probably want to write the index to a temp file in the same folder (as to not cross file system boundaries) first and only if that completes successfully then atomically move it to the final destination - otherwise if you hit ctrl+c during the build a restart would try to load the partially written file.
(If you are doing it that way then ignore my remark :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] kinow commented on a change in pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #1204:
URL: https://github.com/apache/jena/pull/1204#discussion_r825546503
##########
File path: jena-geosparql/src/main/java/org/apache/jena/geosparql/configuration/GeoSPARQLConfig.java
##########
@@ -271,6 +271,19 @@ public static final void setupSpatialIndex(Dataset dataset) throws SpatialIndexE
SpatialIndex.buildSpatialIndex(dataset);
}
+ /**
+ * Setup the precomputed Spatial Index using Dataset Dataset.<br>
Review comment:
s/Setup/Set up, and s/Dataset Dataset/Dataset ? Or Spatial Dataset?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] Aklakan commented on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
Aklakan commented on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1062209248
Just a quick comment for robustness: You probably want to write the index to a temp file first and only if that completes successfully then copy it to the final file - otherwise if you hit ctrl+c during the build a restart would try to load the partially written file.
(If you are doing it like this anyway then ignore my remark :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1074228301
Is there consensus here? What is the next step here?
A name that describes exactly what happens looks better to me.
`GeoSPARQLConfig.setupPrecomputedSpatialIndex(dataset, spatialIndexPath.toFile());`
The question is what is its implementation. Is it `setupSpatialIndex(dataset, null, spatialIndexPath.toFile())` or
```
SpatialIndex si = SpatialIndex.load(spatialIndexFile);
SpatialIndex.setSpatialIndex(dataset, si);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1052151361
Thanks - how can this be tested? Run twice, no configuration changes, first time to build the index and second to use the previously built index?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] LorenzBuehmann commented on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
LorenzBuehmann commented on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1057732717
I'm not sure, but `setupSpatialIndex` does always scan for all SRS URIs first, I don't think this is efficient as it will increase the startup time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] Aklakan edited a comment on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
Aklakan edited a comment on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1062209248
Just a quick comment for robustness: You probably want to write the index to a temp file first and only if that completes successfully then atomically move (Files.move with atomic flag) it to the final destination - otherwise if you hit ctrl+c during the build a restart would try to load the partially written file.
(If you are doing it that way then ignore my remark :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] LorenzBuehmann edited a comment on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
LorenzBuehmann edited a comment on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1077353170
> The question is what is its implementation. Is it `setupSpatialIndex(dataset, null, spatialIndexPath.toFile())`
well, this method would work indeed and is currently unused anyways - what we don't want is to call
`GeoSPARQLConfig.setupSpatialIndex(dataset, spatialIndexPath.toFile());` to avoid scanning for SRS URIs again.
Shall I remove my custom method and just use this method with `null` as SRS argument?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] LorenzBuehmann edited a comment on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
LorenzBuehmann edited a comment on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1057732717
@galbiston
I'm not sure, but `setupSpatialIndex` does always scan for all SRS URIs first, I don't think this is efficient as it will increase the startup time.
But yes, I was thinking the same way first. Maybe setting SRS to `null` then would be better?
```java
// file given but doesn't exist or empty -> compute and serialize index
Path spatialIndexPath = Path.of(spatialIndex);
if ( ! Files.exists(spatialIndexPath) || Files.size(spatialIndexPath) == 0 ) {
GeoSPARQLConfig.setupSpatialIndex(dataset, spatialIndexPath.toFile());
return;
}
// load and setup the precomputed index
GeoSPARQLConfig.setupSpatialIndex(dataset, null, spatialIndexPath.toFile());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] Aklakan edited a comment on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
Aklakan edited a comment on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1062209248
Just a quick comment for robustness: You probably want to write the index to a temp file in the same folder first (as to not cross file system boundaries) and only if that completes successfully then atomically move it to the final destination - otherwise if you hit ctrl+c during the build a restart would try to load the partially written file.
(If you are doing it that way then ignore my remark :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1066194591
Mentioned on #1225.
I find it hard to jump from one area of Jena to another -
Could someone please test this PR and see if it works? Or better, add a test to the test suite?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] LorenzBuehmann edited a comment on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
LorenzBuehmann edited a comment on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1055153216
well, technically just providing an non-empty precomputed index file + querying would trigger the error or with the fix non trigger it hopefully.
But yes, reproduction via assembler would basically take two runs, the first would generate and save the spatial index, in the second run it would simply load the index and register it to the dataset context such that when querying the geospatial extension can make use of it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] LorenzBuehmann edited a comment on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
LorenzBuehmann edited a comment on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1057732717
@galbiston
I'm not sure, but `setupSpatialIndex` does always scan for all SRS URIs first, I don't think this is efficient as it will increase the startup time.
But yes, I was thinking the same way first. Maybe setting SRS to `null` then would be better? There is already an unused method `setupSpatialIndex(Dataset dataset, String srsURI, File spatialIndexFile)` in code, that one could work
```java
// file given but doesn't exist or empty -> compute and serialize index
Path spatialIndexPath = Path.of(spatialIndex);
if ( ! Files.exists(spatialIndexPath) || Files.size(spatialIndexPath) == 0 ) {
GeoSPARQLConfig.setupSpatialIndex(dataset, spatialIndexPath.toFile());
return;
}
// load and setup the precomputed index
GeoSPARQLConfig.setupSpatialIndex(dataset, null, spatialIndexPath.toFile());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] LorenzBuehmann commented on pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
LorenzBuehmann commented on pull request #1204:
URL: https://github.com/apache/jena/pull/1204#issuecomment-1077353170
> The question is what is its implementation. Is it `setupSpatialIndex(dataset, null, spatialIndexPath.toFile())`
well, this method would work indeed and is currently unused anyways - what we don't want is to call
`GeoSPARQLConfig.setupSpatialIndex(dataset, spatialIndexPath.toFile());` to avoid scanning for SRS URIs again.
Shall I remove my custom method and just use this method with `null` as SRS argument?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs merged pull request #1204: JENA-2289: fix to setup the precomputed geospatial index properly
Posted by GitBox <gi...@apache.org>.
afs merged pull request #1204:
URL: https://github.com/apache/jena/pull/1204
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org