-
Notifications
You must be signed in to change notification settings - Fork 329
Start on better indexing support #2223
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
Changes from all commits
a2b64f8
cb6f08b
52d6166
acd6d84
c3885d2
1fd9171
57fb25f
52deaf2
3a8d44a
3ccb22b
585324d
e52c95b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
package common | ||
|
||
import "github.com/authzed/spicedb/pkg/datastore/queryshape" | ||
|
||
// IndexDefinition is a definition of an index for a datastore. | ||
type IndexDefinition struct { | ||
// Name is the unique name for the index. | ||
Name string | ||
|
||
// ColumnsSQL is the SQL fragment of the columns over which this index will apply. | ||
ColumnsSQL string | ||
|
||
// Shapes are those query shapes for which this index should be used. | ||
Shapes []queryshape.Shape | ||
|
||
// IsDeprecated is true if this index is deprecated and should not be used. | ||
IsDeprecated bool | ||
} | ||
|
||
// matchesShape returns true if the index matches the given shape. | ||
func (id IndexDefinition) matchesShape(shape queryshape.Shape) bool { | ||
for _, s := range id.Shapes { | ||
if s == shape { | ||
return true | ||
} | ||
} | ||
return false | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package common | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/authzed/spicedb/pkg/datastore/queryshape" | ||
) | ||
|
||
func TestMatchesShape(t *testing.T) { | ||
id := IndexDefinition{ | ||
Shapes: []queryshape.Shape{queryshape.CheckPermissionSelectDirectSubjects}, | ||
} | ||
|
||
require.True(t, id.matchesShape(queryshape.CheckPermissionSelectDirectSubjects)) | ||
require.False(t, id.matchesShape(queryshape.CheckPermissionSelectIndirectSubjects)) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,14 +39,69 @@ type closeRows interface { | |
Close() | ||
} | ||
|
||
func runExplainIfNecessary[R Rows](ctx context.Context, builder RelationshipsQueryBuilder, tx Querier[R], explainable datastore.Explainable) error { | ||
vroldanbet marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if builder.SQLExplainCallbackForTest == nil { | ||
return nil | ||
} | ||
|
||
// Determine the expected index names via the schema. | ||
expectedIndexes := builder.Schema.expectedIndexesForShape(builder.queryShape) | ||
|
||
// Run any pre-explain statements. | ||
for _, statement := range explainable.PreExplainStatements() { | ||
vroldanbet marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err := tx.QueryFunc(ctx, func(ctx context.Context, rows R) error { | ||
rows.Next() | ||
return nil | ||
}, statement); err != nil { | ||
return fmt.Errorf(errUnableToQueryRels, err) | ||
} | ||
} | ||
|
||
// Run the query with EXPLAIN ANALYZE. | ||
sqlString, args, err := builder.SelectSQL() | ||
if err != nil { | ||
return fmt.Errorf(errUnableToQueryRels, err) | ||
} | ||
|
||
explainSQL, explainArgs, err := explainable.BuildExplainQuery(sqlString, args) | ||
if err != nil { | ||
return fmt.Errorf(errUnableToQueryRels, err) | ||
} | ||
|
||
err = tx.QueryFunc(ctx, func(ctx context.Context, rows R) error { | ||
explainString := "" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use a StringBuilder since this could be a pretty long explain There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is only done during testing; we don't really care about performance in that case There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's death by a thousand cuts. Then we start asking ourselves "why is CI taking so long?". I think keeping tests as fast as the critical path is also important. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I disagree; sometimes you don't want to hyper optimize code (making it less readable) simply to ensure some small improvements on CI. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think using a |
||
for rows.Next() { | ||
var explain string | ||
if err := rows.Scan(&explain); err != nil { | ||
return fmt.Errorf(errUnableToQueryRels, fmt.Errorf("scan err: %w", err)) | ||
} | ||
explainString += explain + "\n" | ||
} | ||
if explainString == "" { | ||
return fmt.Errorf("received empty explain") | ||
} | ||
|
||
return builder.SQLExplainCallbackForTest(ctx, sqlString, args, builder.queryShape, explainString, expectedIndexes) | ||
}, explainSQL, explainArgs...) | ||
if err != nil { | ||
return fmt.Errorf(errUnableToQueryRels, err) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// QueryRelationships queries relationships for the given query and transaction. | ||
func QueryRelationships[R Rows, C ~map[string]any](ctx context.Context, builder RelationshipsQueryBuilder, tx Querier[R]) (datastore.RelationshipIterator, error) { | ||
func QueryRelationships[R Rows, C ~map[string]any](ctx context.Context, builder RelationshipsQueryBuilder, tx Querier[R], explainable datastore.Explainable) (datastore.RelationshipIterator, error) { | ||
span := trace.SpanFromContext(ctx) | ||
sqlString, args, err := builder.SelectSQL() | ||
if err != nil { | ||
return nil, fmt.Errorf(errUnableToQueryRels, err) | ||
} | ||
|
||
if err := runExplainIfNecessary(ctx, builder, tx, explainable); err != nil { | ||
return nil, err | ||
} | ||
|
||
var resourceObjectType string | ||
var resourceObjectID string | ||
var resourceRelation string | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
package common | ||
|
||
import ( | ||
"context" | ||
"testing" | ||
|
||
sq "github.com/Masterminds/squirrel" | ||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/authzed/spicedb/pkg/datastore" | ||
"github.com/authzed/spicedb/pkg/datastore/options" | ||
"github.com/authzed/spicedb/pkg/datastore/queryshape" | ||
) | ||
|
||
type fakeQuerier struct { | ||
queriesRun []string | ||
} | ||
|
||
func (fq *fakeQuerier) QueryFunc(ctx context.Context, f func(context.Context, Rows) error, sql string, args ...interface{}) error { | ||
fq.queriesRun = append(fq.queriesRun, sql) | ||
return nil | ||
} | ||
|
||
type fakeExplainable struct{} | ||
|
||
func (fakeExplainable) BuildExplainQuery(sql string, args []any) (string, []any, error) { | ||
return "SOME EXPLAIN QUERY", nil, nil | ||
} | ||
|
||
func (fakeExplainable) ParseExplain(explain string) (datastore.ParsedExplain, error) { | ||
return datastore.ParsedExplain{}, nil | ||
} | ||
|
||
func (fakeExplainable) PreExplainStatements() []string { | ||
return []string{"SELECT SOMETHING"} | ||
} | ||
|
||
func TestRunExplainIfNecessaryWithoutEnabled(t *testing.T) { | ||
fq := &fakeQuerier{} | ||
|
||
err := runExplainIfNecessary(context.Background(), RelationshipsQueryBuilder{}, fq, fakeExplainable{}) | ||
require.Nil(t, err) | ||
require.Nil(t, fq.queriesRun) | ||
} | ||
|
||
func TestRunExplainIfNecessaryWithEnabled(t *testing.T) { | ||
fq := &fakeQuerier{} | ||
|
||
schema := NewSchemaInformationWithOptions( | ||
WithRelationshipTableName("relationtuples"), | ||
WithColNamespace("ns"), | ||
WithColObjectID("object_id"), | ||
WithColRelation("relation"), | ||
WithColUsersetNamespace("subject_ns"), | ||
WithColUsersetObjectID("subject_object_id"), | ||
WithColUsersetRelation("subject_relation"), | ||
WithColCaveatName("caveat"), | ||
WithColCaveatContext("caveat_context"), | ||
WithColExpiration("expiration"), | ||
WithPlaceholderFormat(sq.Question), | ||
WithPaginationFilterType(TupleComparison), | ||
WithColumnOptimization(ColumnOptimizationOptionStaticValues), | ||
WithNowFunction("NOW"), | ||
) | ||
|
||
filterer := NewSchemaQueryFiltererForRelationshipsSelect(*schema, 100) | ||
filterer = filterer.FilterToResourceID("test") | ||
|
||
builder := RelationshipsQueryBuilder{ | ||
Schema: *schema, | ||
SQLExplainCallbackForTest: func(ctx context.Context, sql string, args []any, shape queryshape.Shape, explain string, expectedIndexes options.SQLIndexInformation) error { | ||
return nil | ||
}, | ||
filteringValues: filterer.filteringColumnTracker, | ||
baseQueryBuilder: filterer, | ||
} | ||
|
||
err := runExplainIfNecessary(context.Background(), builder, fq, fakeExplainable{}) | ||
require.Nil(t, err) | ||
require.Equal(t, fq.queriesRun, []string{"SELECT SOMETHING", "SOME EXPLAIN QUERY"}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
package common | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/authzed/spicedb/pkg/datastore/queryshape" | ||
) | ||
|
||
func TestExpectedIndexesForShape(t *testing.T) { | ||
schema := SchemaInformation{ | ||
Indexes: []IndexDefinition{ | ||
{ | ||
Name: "idx1", | ||
Shapes: []queryshape.Shape{ | ||
queryshape.CheckPermissionSelectDirectSubjects, | ||
}, | ||
}, | ||
{ | ||
Name: "idx2", | ||
Shapes: []queryshape.Shape{ | ||
queryshape.CheckPermissionSelectDirectSubjects, | ||
queryshape.CheckPermissionSelectIndirectSubjects, | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
expectedIndexes := schema.expectedIndexesForShape(queryshape.Unspecified) | ||
require.Empty(t, expectedIndexes) | ||
|
||
expectedIndexes = schema.expectedIndexesForShape(queryshape.CheckPermissionSelectDirectSubjects) | ||
require.Equal(t, []string{"idx1", "idx2"}, expectedIndexes.ExpectedIndexNames) | ||
|
||
expectedIndexes = schema.expectedIndexesForShape(queryshape.CheckPermissionSelectIndirectSubjects) | ||
require.Equal(t, []string{"idx2"}, expectedIndexes.ExpectedIndexNames) | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.