RegionTermIndexer: Port from C++ #178

sreekanth-cb · 2025-05-14T03:41:50Z

Attempt to port a functional version of S2RegionTermIndexer

google-cla · 2025-05-14T03:41:54Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

rsned · 2025-05-14T03:58:13Z

s2/region_term_indexer.go

+// -----------------
+//
+// Given a query region, we want to find all of the document regions that
+// intersect it.  The first step is to represent all the regions as S2Cell


Throughout, use the Go type names instead of the C++ type names. (e.g Cell vs S2Cell)

rsned · 2025-05-14T04:00:09Z

s2/region_term_indexer.go

+var defaultMaxCells = int(8)
+
+type Options struct {
+	///////////////// Options Inherited From S2RegionCoverer ////////////////


Go doesn't inherit. Can say see RegionCoverer for more details on these options.

rsned · 2025-05-14T04:14:16Z

s2/region_term_indexer.go

+	return &RegionTermIndexer{options: option}
+}
+
+func (rti *RegionTermIndexer) GetTerm(termTyp TermType, id CellID,


You can just use a single letter for the type 'r' vs 'rti' in the method reciever.

rsned · 2025-05-14T04:17:08Z

s2/region_term_indexer.go

+var marker = string('$')
+
+const (
+	ANCESTOR TermType = iota + 1


Preface this enum values with the type so we know what it's for when we see it. e.g., TermTypeAncestor.

iota is sufficient, there's no need to start it at +1.

rsned · 2025-05-14T04:18:21Z

s2/region_term_indexer.go

+	// covering term, but as an optimization we always index these cells as
+	// ancestor terms only.  This is possible because query regions will never
+	// contain a descendant of such cells.  Note that this is true even when
+	// max_level() != true_max_level() (see S2RegionCoverer::Options).


update these methods to match the naming they have above.

rsned · 2025-05-14T06:15:42Z

s2/region_term_indexer.go

@@ -0,0 +1,441 @@
+// Copyright 2025 Google Inc. All rights reserved.


Change this from "Google Inc." to "The S2 Geometry Project Authors."

It doesn't look like we've done that in any files yet, though it may be a good idea.

The copyright story is perhaps complicated because a lot of this file is copied verbatim from a C++ file which has a Google Inc copyright line.

rsned · 2025-05-14T06:18:11Z

s2/region_term_indexer_test.go

+	}
+}
+
+func TestIndexRegionsQueryRegionsOptimizeTime(t *testing.T) {


Following the style of other test names in this package, preface the test case name with the file/type here to make it clearer where a failing test is coming from. e.g.
TestRegionTermIndexer......

rsned · 2025-05-14T06:32:23Z

s2/region_term_indexer_test.go

+func TestIndexPointsQueryRegionsOptimizeTime(t *testing.T) {
+	options := Options{minLevel: 0,
+		levelMod:      2,
+		maxLevel:      30,


Should use the actual MaxLevel constant here and throughout.

rsned · 2025-05-14T06:33:37Z

s2/region_term_indexer_test.go

+		maxCells:      8,
+	}
+	testRandomCaps(t, options, CAP)
+}


What about the MaxLevelSetLoosely and MarkerCharacter tests?

rsned · 2025-05-14T06:40:18Z

s2/region_term_indexer.go

+	// DEFAULT: false
+	optimizeSpace bool
+}
+


The fact we need to explicitly set levelMod on every unit test feels like a point of trouble that's going to bite people down the road. A default options instance should work safely without the user needing to know to set certain fields.

e.g. Anyone who forgets to set it will get stuck with infinite loops because several methods do this:

for (level - rti.options.levelMod) >= rti.options.minLevel {

I think there should be a DefaultRegionTermIndexerOptions() that gives the caller a safe variable to start from when they need an options.

There will still be a foot gun. @alan-strohm @flwyd Thoughts on idiomatic ways to check for valid options?

Functional options would be a good pattern here. (See the go-cmp project for a good example of Options in use.)

I also question whether the Options type needs to be public: could there be a NewRegionTermIndexer constructor that takes a list of RegionTermIndexerOption functions which set state in the RegionTermIndexer and the RegionCoverer? It looks like most of the fields in Options just get copied over to the Coverer.

Do users of the indexer need to be able to inspect the option state (public fields), or would a set-only interface, as in the functional options pattern, suffice?

Do users of the indexer need to be able to inspect the option state (public fields), or would a set-only interface, as in the functional options pattern, suffice?

The accessors are only so the builder and related functions can get the options. Clients only need to set them.

jmr · 2025-05-14T11:51:23Z

There are lint errors: https://github.com/golang/geo/actions/runs/15011742506/job/42186000244?pr=178#step:4:34

jmr · 2025-05-14T12:10:59Z

It looks like this is a copy of https://github.com/blevesearch/geo/commits/master/s2/region_term_indexer.go [helping to resolve #134] but you wrote that anyway, then changed email addresses. It would save time to provide a bit more background in the PR.

jmr · 2025-05-14T12:12:56Z

s2/region_term_indexer.go

+	// indexes but want fast serving, it might be reasonable to set
+	// max_cells() == 100 during indexing and max_cells() == 8 for queries.
+	//
+	// DEFAULT: 8  (coarse approximations)


"DEFAULT" is a bit misleading here. Document the value in DefaultRegionTermIndexerOptions and reference that instead?

flwyd · 2025-05-17T06:55:17Z

s2/region_term_indexer.go

+// limitations under the License.
+
+//
+// Indexing Strategy


Put the RegionTermIndexer type (and its documentation) at the top so that someone browsing this file can quickly understand what it's about. This comment is an implementation detail coming from the .cc file; having the code that's ported from .h first is a more natural introduction to the file. That move will also help the reader understand terms like "document" in this comment.

Also nit: This long block comment might read a little nicer with /* … */ rather than line-comments. (Starting each line with * isn't needed.)

flwyd · 2025-05-17T07:04:41Z

s2/region_term_indexer.go

@@ -0,0 +1,441 @@
+// Copyright 2025 Google Inc. All rights reserved.


It doesn't look like we've done that in any files yet, though it may be a good idea.

The copyright story is perhaps complicated because a lot of this file is copied verbatim from a C++ file which has a Google Inc copyright line.

flwyd · 2025-05-17T07:08:09Z

s2/region_term_indexer.go

+	return trueMax
+}
+
+// RegionTermIndexer is a helper struct for adding spatial data to an


"helper class" is a term that's popular in the C++ world that I don't find particularly informative. Perhaps "RegionTermIndexer is a tool for adding spatial data…" or "RegionTermIndexer adds spatial data…"

flwyd · 2025-05-17T07:09:02Z

s2/region_term_indexer.go

+// and then building an "inverted index" that maps each term to a list of
+// documents (and document positions) where that term occurs.
+//
+// This class deals with the problem of converting spatial data into index


Go doesn't have classes.

flwyd · 2025-05-17T07:14:23Z

s2/region_term_indexer.go

+// terms, which can then be indexed along with the other document information.
+//
+// Spatial data is represented using the S2Region type.  Useful S2Region
+// subtypes include:


The documentation for this type will be on the same page as the docs for s2.Region and all these implementing types. Is it sufficient to say this indexer works with any type that implements the Region interface?

flwyd · 2025-05-17T08:11:26Z

s2/region_term_indexer_test.go

+		indexTerms += len(terms)
+	}
+
+	for i := 0; i < NUM_ITERATIONS; i++ {


These loops look very similar. Add a comment that explains the difference in some way.

flwyd · 2025-05-17T08:12:28Z

s2/region_term_indexer_test.go

+		sort.Ints(expected)
+		sort.Ints(actual)
+
+		if !reflect.DeepEqual(expected, actual) {


This looks like a case for cmp.Diff

flwyd · 2025-05-17T08:13:17Z

s2/region_term_indexer_test.go

+			t.Errorf("expected: %+v Vs actual: %+v", expected, actual)
+		}
+
+		queryTerms += len(terms)


Is this variable ever read?

flwyd · 2025-05-17T08:16:19Z

s2/region_term_indexer_test.go

+
+const (
+	POINT QueryType = iota + 1
+	CAP


Would it be useful to test Region types other than Cap?

flwyd · 2025-05-17T08:21:00Z

s2/region_term_indexer_test.go

+		deduplicate(&actual)
+
+		sort.Ints(expected)
+		sort.Ints(actual)


Alternatively, you could ensure uniqueness by using map[int]struct{} (or bool) rather than a slice.

Introducing RegionTermIndexer

50d7cb1

rsned reviewed May 14, 2025

View reviewed changes

jmr changed the title ~~Introducing RegionTermIndexer~~ RegionTermIndexer: Port from C++ May 14, 2025

jmr reviewed May 14, 2025

View reviewed changes

flwyd requested changes May 17, 2025

View reviewed changes

		@@ -0,0 +1,441 @@
		// Copyright 2025 Google Inc. All rights reserved.

RegionTermIndexer: Port from C++ #178

Are you sure you want to change the base?

RegionTermIndexer: Port from C++ #178

Conversation

sreekanth-cb commented May 14, 2025

Uh oh!

google-cla bot commented May 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmr commented May 14, 2025

Uh oh!

jmr commented May 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!