Repository navigation
[scd] Make repo search/notify methods cells-native - #1711
Conversation
9a65891 to
146fb77
Compare
mickmis
left a comment
mickmis
left a comment
There was a problem hiding this comment.
I think it's a great simplification, that can actually be taken a step further, see my comments. At the end of the day I don't think the DSS ever has to process the geometries of the Volume4D directly?
So LGTM modulo comments.
| } | ||
|
|
||
| // Map iteration order is random and some CellUnion methods like Contains require a sorted union. | ||
| sort.Slice(result, func(i, j int) bool { return result[i] < result[j] }) |
There was a problem hiding this comment.
Sounds like this should be a guarantee of CalculateCovering. If you agree can you split that up in a separate PR, implement it in other implementations, and add this to the interface doc?
In addition, this should be covered by unit tests.
5370c95 to
640eb15
Compare
34f6bfc to
8a59499
Compare
mickmis
left a comment
mickmis
left a comment
There was a problem hiding this comment.
LGTM, just a few minor comments. This is a nice improvement!
| result.TimeStart = restapi.Time{ | ||
| Value: o.StartTime.Format(time.RFC3339Nano), | ||
| Format: TimeFormatRFC3339, | ||
| if o.CellsVolume4D != nil { |
There was a problem hiding this comment.
Since CellsVolume4D is embedded, you don't actually need to check for it being nil when accessing its fields. Demo: https://go.dev/play/p/0g1bLtymQNw
Applies to all others too
There was a problem hiding this comment.
Awesome ! I didn't know about this :)
| return nil, stacktrace.Propagate(err, "Error constructing 4D volumes union") | ||
| } | ||
| // Compute total affected cellsVolume for notification purposes | ||
| notifyCellsVol4D := validParams.volume |
8a59499 to
5724180
Compare
5724180 to
d268550
Compare
2 participants
Following this discussion under #1670, this PR makes the SCD search and increment repo methods take a cells based volume instead of the current
*dssmodels.Volume4D. This way, the backends no longer recompute the S2 covering themselves and this is done at the http handler level. Eventually, this will allow us to pre-compute the S2 cells covering before proposing to Raft to avoid floating point arithmetics related to geometry on the apply path.The PR also adds a missing
sortinprecomputedCellGeometry.CalculateCovering().Implements #1701