feat: Secondary index reader by garrensmith · Pull Request #829 · tigrisdata-archive/tigrisdb

garrensmith · 2023-02-16T10:46:11Z

Describe your changes

Secondary Index reader that queries from the secondary index.

How best to test these changes

Issue ticket number and link

garrensmith · 2023-02-22T08:49:34Z

+	}
+
+	// TODO: I think this might need to be changed, we should only support search on the search endpoint
 	if options.filter.None() || !options.filter.IsIndexed() {


Do we still support search as a reader option or should it only be part of the search API?

The reader option is internal to us but to answer your question, yes we need to support it till we backfill the secondary indexes and point the read queries to start using secondary indexes. Then it will only be Search.

Created issue #869 for this

himank · 2023-02-22T17:10:19Z

 	SecondaryIndex: SecondaryIndexConfig{
-		ReadEnabled:  false,
-		WriteEnabled: false,
+		ReadEnabled:  true,


let's not enable it in the config yet.

himank · 2023-02-22T20:29:07Z

+	}
+
+	// TODO: I think this might need to be changed, we should only support search on the search endpoint
 	if options.filter.None() || !options.filter.IsIndexed() {


The reader option is internal to us but to answer your question, yes we need to support it till we backfill the secondary indexes and point the read queries to start using secondary indexes. Then it will only be Search.

himank · 2023-02-23T04:30:26Z

+	eqPlan, err := eqKeyBuilder.Build(queryFilters, coll.QueryableFields)
+	if err == nil {
+		for _, plan := range eqPlan {
+			if indexedDataType(plan) {


if it is equality and the plan is not index type what is the behavior?

This can happen if the field is a []byte. Then the plan is ignored and we look for another eq plan or move to range plans

himank · 2023-02-23T05:23:55Z

 		if len(repeatedFields) == 0 {
-			// nothing found or a gap
-			return nil, errors.InvalidArgument("filters doesn't contains primary key fields")
+			if s.matchAll {


So this will go away once we move to secondary indexes because then it doesn't matter if we are able to build the primary key from the filter.

Yes that will be awesome. I've created #870 to remember to do this.

himank · 2023-02-23T05:45:41Z

+	}
+
+	// Prefer a RANGE scan which should read back less keys than a FULLRANGE
+	// for _, queryType := range []filter.QueryPlanType{filter.RANGE, filter.FULLRANGE} {


remove it if not needed.

himank · 2023-02-23T06:07:50Z

+// Range Key Composer will generate a range key set on the user defined keys
+// It will set the KeyQuery to `FullRange` if the start or end key is not defined in the query
+// if there is a defined start and end key for a range then `Range` is set.
+type RangeKeyComposer[F fieldable] struct {


For this diff, the structure is good. But if we think about it, we just need two high level abstractions here,

NewQueryableKeyBuilder(composer, primaryKey) [will be used by insert/replace only]

NewQueryableKeyBuilder(): This will internally use eq/range composer to build plans i.e. equality as well as range. The reason being once we don't rely on the search store then either the plan is formed or we need to fall back to full table scan, there will be no other option. Then Read/Update/Delete can rely directly on it. This means we need to add something to the QueryPlan so that caller knows whether the keys are for primary or secondary indexes.

Yes I like it. We can definitely move to that direction once search and secondary are ready.

garrensmith

Thanks for the feedback. I've made the changes.

garrensmith · 2023-02-24T08:35:42Z

+}
+
+// NewPrimaryKeyEQBuild returns a KeyBuilder for use with schema.Field to build a primary key query plan.
+func NewPrimaryKeyEqBuilder(keyEncodingFunc KeyEncodingFunc) *KeyBuilder[*schema.Field] {


I think we could do that a little later once secondary indexes are deployed. At the moment we have to pass some extra params if it is a PrimaryKey versus secondary.

garrensmith · 2023-02-24T08:37:49Z

 		if len(repeatedFields) == 0 {
-			// nothing found or a gap
-			return nil, errors.InvalidArgument("filters doesn't contains primary key fields")
+			if s.matchAll {


Yes that will be awesome. I've created #870 to remember to do this.

garrensmith · 2023-02-24T08:40:00Z

+// Range Key Composer will generate a range key set on the user defined keys
+// It will set the KeyQuery to `FullRange` if the start or end key is not defined in the query
+// if there is a defined start and end key for a range then `Range` is set.
+type RangeKeyComposer[F fieldable] struct {


Yes I like it. We can definitely move to that direction once search and secondary are ready.

garrensmith · 2023-02-24T08:41:16Z

+	eqPlan, err := eqKeyBuilder.Build(queryFilters, coll.QueryableFields)
+	if err == nil {
+		for _, plan := range eqPlan {
+			if indexedDataType(plan) {


This can happen if the field is a []byte. Then the plan is ignored and we look for another eq plan or move to range plans

himank · 2023-02-28T05:16:42Z

 		}
 		collation = value.NewCollationFrom(apiCollation)
+
+		if buildForSecondaryIndex && collation.IsCaseInsensitive() {


So we won't be supporting case-insensitive queries on secondary indexes fields?

We can support it in the in-memory filter part but not part of what we use to build the range keys. Or at least I can't think of a way of making that work.

Sure, we can figure it out iteratively.

Supports $eq, $lt, $lte, $gt and $gte queries from the secondary index. The secondary index can be used for reads, updates and deletes.

tigrisdata-argocd-bot · 2023-03-03T14:36:45Z

🎉 This PR is included in version 1.0.0-beta.46 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

reviewpad bot added the large label Feb 16, 2023

garrensmith added debug image This PR will generate a debug image and removed large labels Feb 16, 2023

reviewpad bot added the large label Feb 16, 2023

garrensmith force-pushed the secondary-reader branch from 6b78318 to 5d5a089 Compare February 21, 2023 18:05

garrensmith commented Feb 22, 2023

View reviewed changes

garrensmith force-pushed the secondary-reader branch from 5d5a089 to 7a925db Compare February 22, 2023 14:31

garrensmith commented Feb 22, 2023

View reviewed changes

Comment thread query/filter/key_builder.go

garrensmith force-pushed the secondary-reader branch from 7a925db to d7051f1 Compare February 22, 2023 14:40

himank reviewed Feb 23, 2023

View reviewed changes

garrensmith changed the title ~~[WIP]Secondary reader~~ Secondary reader Feb 23, 2023

garrensmith commented Feb 24, 2023

View reviewed changes

garrensmith force-pushed the secondary-reader branch from 0eb65ee to 2726177 Compare February 27, 2023 18:00

himank reviewed Feb 28, 2023

View reviewed changes

garrensmith force-pushed the secondary-reader branch 3 times, most recently from 0896d17 to 74869e7 Compare March 1, 2023 05:54

himank previously approved these changes Mar 1, 2023

View reviewed changes

himank changed the title ~~Secondary reader~~ feat: Secondary index reader Mar 1, 2023

garrensmith added 6 commits March 1, 2023 10:12

feat: Top level reads from secondary index

85f4a3d

Supports $eq, $lt, $lte, $gt and $gte queries from the secondary index. The secondary index can be used for reads, updates and deletes.

fix: fixes for document collation tests

96b749f

fix: fixes from review

de5d58c

fix: do not index on empty filter

9f6429b

fix: ignore for secondary

1227db9

fix: add config to use secondary for mutations

76374a1

garrensmith dismissed himank’s stale review via 76374a1 March 1, 2023 08:12

garrensmith force-pushed the secondary-reader branch from 74869e7 to 76374a1 Compare March 1, 2023 08:12

adilansari approved these changes Mar 1, 2023

View reviewed changes

garrensmith merged commit 40f2fdd into main Mar 1, 2023

garrensmith deleted the secondary-reader branch March 1, 2023 10:39

tigrisdata-argocd-bot added the released on @beta label Mar 3, 2023

Conversation

garrensmith commented Feb 16, 2023

Describe your changes

How best to test these changes

Issue ticket number and link

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

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!

garrensmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

tigrisdata-argocd-bot commented Mar 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants