-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Import GeoWave Indexing Code #20
base: master
Are you sure you want to change the base?
Conversation
Taken from GeoWave commit 73ae05af4358d5c8741963eb692a9189ebf616b9. Signed-off-by: James McClain <[email protected]>
From 73ae05af4358d5c8741963eb692a9189ebf616b9
6bf8df1
to
1d420ba
Compare
* Version 2.0 which accompanies this distribution and is available at | ||
* http://www.apache.org/licenses/LICENSE-2.0.txt | ||
******************************************************************************/ | ||
package org.locationtech.sfcurve.geowave.geotime.index.dimension; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd opine that naming any of our projects in the package names is inappropriate. We haven't done it for GeoMesa or GeoTrellis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this, but also the GeoWave code is another implementation of Hilbert. What's unique about this approach is that it takes GeoWave's strategy specifically. I think it will make it a lot easier to reference and have everyone know what we are talking about. GeoTrellis's Z curve implementation was the original Z curve, and was only a Z curve - it didn't include a lot of the bells and whistles that the geowave-index project contains (tiering, periodicity). GeoMesa's bells and whistles still live in GeoMesa - you are using the baseline Z curve implementations as they've been defined and modified here.
If there's a better name for it than something like hilbert2
, I'm up for a better option. It doesn't make me uncomfortable to reference GeoWave from the codebase, however, especially since the subproject is a straight up pull from geowave-index (in our PR for GeoTrellis, we have also labelled the indexing strategy that leans on this as GeoWave).
/cc @rfecher, not sure if putting GeoWave into the naming makes sense to you, or if you know of a better name that you'd like more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me @lossyrob -- keep the name geowave
or perhaps hilbert-geowave
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going back to this, the underlying Hilbert implementation is the Uzaygezen library.;)
@pomadchin sweet! Looks like there's a quick merge conflict to resolve. Can you or @lossyrob knock that out? |
@jnh5y sure! |
@jnh5y probably it's ready for your review |
I'm rethinking our approach here. GeoWave's initial IP contribution has been approved by the IP team at Eclipse, so the IP considerations are no longer an issue. However, in the meantime this PR has gotten pretty stale as GeoWave's The original purpose of this PR was to get GeoWave indexing code into SFCurve so we could A. rely on the geowave indexing code in GeoTrellis and B. gain a dependency on SFCurve from GeoTrellis, which would push the SFCurve story along. Considering the effort it would take to keep this SFCurve GeoWave code with the Thoughts? @echeipesh @rfecher @jnh5y |
Agreed, it really only works if each of the projects are depending on the same baseline, or else it just ends up being divergent forks. To that end, geowave needs to change its dependency to there's a significant change coming in |
I'm fine with the code moving over whenever. Generally, I'm for moving common pieces out as projects like that. Admittedly, that does require more coordination. For the short term, it might make sense for GeoTrellis to depend on a released version of the GeoWave indexing code. Meanwhile, the GeoWave folks can sort out the changes in the pipeline; once those are ironed out, they can contribute to SFCurve. As that happens, we can cut a quick release so that GW and GT can depend on the moved code. Does that work for everyone? |
Supersedes #18
This PR imports the GeoWave "index" subproject into SFCurve. With this, representatives of all of the GeoWave, GeoMesa and GeoTrellis indexing code will be in this repository. GeoTrellis plans to move to using this indexing by default for it's 2.0 release.
All namespaces have been moved to
org.locationtech.sfcurve.geowave
.With all of our indexing code under one roof, we can then move to the phase where we try to bring it all together under one-api-to-rule-them-all. Pie in the sky? Maybe ☁️
The code in this PR is set for IP review by an Eclipse CQ https://dev.eclipse.org/ipzilla/show_bug.cgi?id=14322