search

locationtech / geowave

GeoWave provides geospatial and temporal indexing on top of Accumulo, HBase, BigTable, Cassandra, Kudu, Redis, RocksDB, and DynamoDB.
Apache License 2.0
502 stars 190 forks source link

Aggregation API and Geohash Aggregation implementation #1733

Closed sinistersnare closed 4 years ago

sinistersnare commented 4 years ago

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 commented 4 years ago

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

jdgarrett commented 4 years ago

@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?

rfecher commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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.

sinistersnare commented 4 years ago

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

Woo!