Aggregation API and Geohash Aggregation implementation by sinistersnare · Pull Request #1733 · locationtech/geowave

sinistersnare · 2020-06-04T19:59:46Z

Hi!

This partly goes towards #1703. After this is in, creating hex-bin aggregations should be easy enough, and that can be closed.

There are a few TODOs left in here, but mostly its done! Just wanted to see what you guys need from me to get this merged in. Also a code-review would be great.

Thanks!

sinistersnare · 2020-06-04T21:08:39Z

I signed the ECA a while ago, how can I get confirmation of that?

jdgarrett · 2020-06-04T20:08:46Z

@@ -0,0 +1,136 @@
+package org.locationtech.geowave.core.geotime.store.query.aggregate;


Missing copyright header.

Should be resolved.

jdgarrett · 2020-06-04T20:12:19Z

+      }
+    }
+    // TODO: should this be 0, AKA 1 bin for all data? (if so make sure to fix tests!)
+    return 1;


I think it makes sense to always have some degree of binning, otherwise there would be no reason to use a binning strategy at all. However, if the max bins allowed is less than 32, I would suggest throwing an IlleagalArgumentException that states that at least 32 bins are required for this strategy.

Should be resolved, I implemented this.

jdgarrett · 2020-06-04T20:13:01Z

+  /**
+   * Create a Binning Strategy based on GeoHashes.
+   *
+   * @param maxBinsAllowed TODO: explain how this invariant works, and how itll break for < 32.


See my next comment about throwing an exception.

Should be resolved.

jdgarrett · 2020-06-04T20:17:28Z

@@ -0,0 +1,149 @@
+package org.locationtech.geowave.core.geotime.store.query.aggregate;


Missing copyright header.

Should be resolved.

jdgarrett · 2020-06-04T20:26:24Z

+   * @param num The number to serialize as a {@link ByteBuffer}
+   * @return a byte array that represents the given BigDecimal.
+   */
+  public static byte[] writeBigDecimal(final BigDecimal num) {


I am not sure these functions really fit in VarintUtils, This class is mostly used for encoding integers using as few bytes possible, but it doesn't look like there is any variable length encoding being used. There is another class that handles BigDecimal serialization at org.locationtech.geowave.core.store.data.field.base.BigDecimalSerializationProvider.

Good idea, I just lifted the code I removed from the math-agg and used that instead of this stuff. However, I changed it slightly so it was a better ByteBuffer citizen (storing the array length, instead of using all of .remaining()

jdgarrett · 2020-06-04T20:57:44Z

+import java.util.List;
+import java.util.Map;
+
+public class GeohashBinningExample {


We need to do a better job of explaining our examples, there should be a comment at the top of this class that explains what the example does.

I added a bunch of comments, let me know if its better, or it should be explained more thoroughly.

jdgarrett · 2020-06-04T20:58:37Z

+            allResults.entrySet().stream().filter((e) -> e.getKey().length() == 6).toArray()));
+  }
+
+  private static Map<String, BigDecimal> executeBinningAggregation(int precision) {


This method should have fairly detailed comments, this will be used as a learning tool.

Same as above.

jdgarrett · 2020-06-04T21:10:07Z

-    if (result == null) {
-      return new byte[0];
-    }
-    final byte[] unscaled = result.unscaledValue().toByteArray();


You can move this logic to the VarintUtils class since it is using Variable length encoding for each part of the BigDecimal.

Talked about above, I moved this code over with some slight changes.

jdgarrett · 2020-06-04T21:13:47Z


 @RunWith(GeoWaveITSuiteRunner.class)
 @SuiteClasses({
-    GeoWaveVisibilityIT.class,


Comment these back in

already done, forgot to uncomment it.

jdgarrett · 2020-06-04T21:16:38Z

@@ -0,0 +1,133 @@
+package org.locationtech.geowave.test.basic;


Missing copyright header.

Should be resolved.

jdgarrett · 2020-06-04T21:26:26Z

@rfecher I noticed when reviewing this that most aggregations simply return new byte[0] in their serialization functions. It the expectation that an aggregation that is serialized and then deserialized is equivalent to the original, including the value?

sinistersnare · 2020-06-04T21:46:13Z

-import com.google.common.collect.Lists;
+import org.opengis.feature.simple.SimpleFeatureType;
+import java.util.Date;
+import java.util.List;


need to go thru the import reorganizations and change them back, they were fallout from a different formatter.

rfecher · 2020-06-05T12:24:32Z

I signed the ECA a while ago, how can I get confirmation of that?

@sinistersnare I wouldn't worry too much about it at this stage, when you complete the PR make sure you sign all your commits with the same email as the ECA and it should pass the ECA validation.

rfecher · 2020-06-05T12:39:27Z

@rfecher I noticed when reviewing this that most aggregations simply return new byte[0] in their serialization functions. It the expectation that an aggregation that is serialized and then deserialized is equivalent to the original, including the value?

@jdgarrett the "Result" and the "Params" are separate from the function itself and serialized separately. I actually would expect all Aggregations to be empty byte serializations (byte[0]), because the input to the function goes in params anyways. I think I had good reason to separate serialization of params and the actual function (there's definitely good reason to separate the result). It seems fairly clean and flexible considering the aggregation function is also responsible for serializing/deserializing result and computing the aggregation, there's no need to inherently tie a potentially heavy input parameter to its serialization. By far the most complicated aggregation at this point in the baseline is distributed rendering so making that make reasonable sense drove some of the design decisions (org.locationtech.geowave.adapter.vector.render.DistributedRenderAggregation).

sinistersnare · 2020-06-18T15:41:03Z

I have addressed most of the comments, still some work to be done.

Me and JD had a talk on some different constructors for the GeohashBinningStrategy. Im not quite sure how I can implement them, and I made comments on that front.

Aside from that, testing, and docs, this is looking better and better.

…ashes. Signed-off-by: Davis Silverman <davis.silverman@maxar.com>

sinistersnare · 2020-07-02T04:20:51Z

Signed my commit, rebased onto master (fixing that last TODO!), now we just wait for CI!

Woo!