Add query filter expressions to easily constrain any supported adapter field #1826
Add query filter expressions to easily constrain any supported adapter field #1826rfecher merged 1 commit intolocationtech:masterfrom
Conversation
rfecher
left a comment
There was a problem hiding this comment.
So far my understanding of where this is is at internal changes to support filter expressions, but I don't see public API changes. Is that fair and if so is that the intent of this PR with a subsequent PR coming for public API changes?
| if (preparedGeometry == null) { | ||
| preparedGeometry = FACTORY.create(GeometryUtils.geometryFromBinary(geometryBinary, null)); | ||
| preparedGeometry = | ||
| PreparedGeometryFactory.prepare(GeometryUtils.geometryFromBinary(geometryBinary, null)); |
There was a problem hiding this comment.
is it primarily aesthetics that we don't use the static FACTORY create instead of this static prepare method? I don't actually think it matters here, but I wonder if there are places we use this factory prepare that could be on a "per row" basis? The only difference between the 2 is the static method does (new PreparedGeometryFactory()).create(geom) and the actual create method may as well have been static because it doesn't have any use of the factory instance. Point being its creating an empty instance which is negligible under most circumstances, but seems like it could be additional garbage collection if done hundreds of thousands or millions of times. Just food for thought (figure referencing a static instance from geometry utils wouldn't hurt but also this prepare methods is unlikely to be done "per row" I suppose).
There was a problem hiding this comment.
I had originally moved the static factory to GeometryUtils, but I switched it to this method because it was only really being called once per query in SpatialQueryFilter and once per literal in the filter expressions, so it seemed silly to hold a static instance around for that.
| import org.locationtech.geowave.test.stability.GeoWaveStabilityIT; | ||
|
|
||
| @RunWith(GeoWaveITSuiteRunner.class) | ||
| @SuiteClasses({ |
There was a problem hiding this comment.
obviously this is still WIP. just wanted to mark this so we don't los track.
| @Override | ||
| public void fromBinary(byte[] bytes) { | ||
| final Geometry unprepared = GeometryUtils.geometryFromBinary(bytes, null); | ||
| geometry = PreparedGeometryFactory.prepare(unprepared); |
There was a problem hiding this comment.
reading the "interning" comment on SpatialQueryFilter I wonder whether this should use something similar like GeometryImage? (it uses it in fromBinary as well as as the constructor, but I don't know for sure how relevant it still is, seems like it could be worthy of a little investigation though based on the claims in performance gains).
Yes, the API changes are still a WIP, this PR was to really to get feedback on the filter expressions themselves and how they are built/work. |
…r field Also selects the best index to query based on row range histogram statistics if no index is specified by the query builder. Signed-off-by: Johnathan Garrett <jd@prominentedge.com>
Also selects the best index to query based on row range histogram statistics if no index is specified by the query builder.