-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Various optimizations for Geo queries #3805
Conversation
…ecute in parallel. Fix up convertGeom to also take geometrys, not just coordinates.
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.
✅ A review job has been created and sent to the PullRequest network.
Check the status or cancel PullRequest code review here.
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.
A couple minor comments but it otherwise.
Reviewed 5 of 5 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @danielmai, @gitlw, and @manishrjain)
types/s2.go, line 152 at r2 (raw file):
Quoted 8 lines of code…
coords := v.Polygon(i).Coords() if len(coords) == 0 { return nil, errors.Errorf("Got empty polygon inside multi-polygon.") } // Check that first ring is closed. if !closed(v.Polygon(i).Coords()[0]) { return nil, errors.Errorf("Last coord not same as first") }
Except for the error message, the logic here and below is the same. Maybe you could remove the duplication but I'll leave it up to you.
worker/task.go, line 1271 at r1 (raw file):
return err } // list type
This comment is not very helpful.
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.
Reviewable status: 4 of 5 files reviewed, 3 unresolved discussions (waiting on @danielmai, @manishrjain, and @martinmr)
types/s2.go, line 152 at r2 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
coords := v.Polygon(i).Coords() if len(coords) == 0 { return nil, errors.Errorf("Got empty polygon inside multi-polygon.") } // Check that first ring is closed. if !closed(v.Polygon(i).Coords()[0]) { return nil, errors.Errorf("Last coord not same as first") }
Except for the error message, the logic here and below is the same. Maybe you could remove the duplication but I'll leave it up to you.
+1 on the suggestion above to have a common method for validating a polygon. But since this is a minor issue, it's up to you.
worker/task.go, line 1273 at r2 (raw file):
return err } // list type
remove this comment since the pl.Iterate would include both t1he list and singular predicate types?
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.
Left minor comments, otherwise
Reviewable status: 4 of 5 files reviewed, 3 unresolved discussions (waiting on @danielmai, @manishrjain, and @martinmr)
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.
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @danielmai, @gitlw, and @martinmr)
types/s2.go, line 152 at r2 (raw file):
Previously, gitlw (Lucas Wang) wrote…
+1 on the suggestion above to have a common method for validating a polygon. But since this is a minor issue, it's up to you.
Done.
worker/task.go, line 1271 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
This comment is not very helpful.
Done.
worker/task.go, line 1273 at r2 (raw file):
Previously, gitlw (Lucas Wang) wrote…
remove this comment since the pl.Iterate would include both t1he list and singular predicate types?
Done.
This change is