Relationships selected in SQL-based datastores now elide columns that have static values #2096

josephschorr · 2024-10-21T23:20:05Z

This vastly reduces data over the wire, as well as deserialization time and memory usage

Fixes #59
Fixes #1527

tstirrat15

See comments. What I saw looked good but I wouldn't mind having another set of eyes on it.

tstirrat15 · 2024-10-22T19:10:35Z

internal/datastore/common/relationships.go

+}
+
+// QueryRelationships queries relationships for the given query and transaction.
+func QueryRelationships[R Rows, C ~map[string]any](ctx context.Context, queryInfo QueryInfo, sqlStatement string, args []any, span trace.Span, tx Querier[R], withIntegrity bool) (datastore.RelationshipIterator, error) {


What does the ~ mean in this context?

~T means the set of all types whose underlying type is T

If you did something like type MyType map[string]any, doing ~map[string]any will also accept that and not just explicitly map[string]any

Which is exactly what we do

tstirrat15 · 2024-10-22T21:52:05Z

internal/datastore/crdb/reader.go

-			colIntegrityKeyID,
-			colIntegrityHash,
-			colTimestamp,


For myself: this was moved into extraFields on the common.SchemaInformation struct.

tstirrat15 · 2024-10-22T22:25:43Z

internal/datastore/mysql/datastore.go

+type wrappedTX struct {
+	tx querier
+}


What role does this play?

It translates the interface

tstirrat15 · 2024-10-22T22:28:02Z

internal/datastore/common/sql.go

+	ColCaveatName         string
+	ColCaveatContext      string
+	PaginationFilterType  PaginationFilterType
+	PlaceholderFolder     sq.PlaceholderFormat


Also is this in here because this allows you to push more logic up into the common sql logic?

tstirrat15 · 2024-10-22T22:34:31Z

internal/datastore/spanner/reader.go

+			var resourceObjectType string
+			var resourceObjectID string
+			var relation string


Does it mean anything that these declarations moved out of the loop?

It means they are on the heap, which isn't good, but it also means we don't need to recalculate them every iteration, which is good

tstirrat15 · 2024-10-22T22:35:00Z

pkg/datastore/test/relationships.go

@@ -1573,10 +1573,7 @@ func ConcurrentWriteSerializationTest(t *testing.T, tester DatastoreTester) {
 			<-waitToFinish
 			return err
 		})
-		if err != nil {
-			panic(err)


I take it this is unnecessary when we're using MustBugF?

This is in a test, so I changed it to just return the error

tstirrat15 · 2024-10-22T22:36:10Z

internal/datastore/common/sql.go

@@ -562,17 +654,57 @@ func (tqs QueryExecutor) ExecuteQuery(
 		limit = *queryOpts.Limit
 	}

-	toExecute := query.limit(limit)
+	if limit < math.MaxInt64 {
+		query = query.limit(limit)


Is this conditional just to reduce what's going over the wire?

Basically, yes

josephschorr · 2024-10-23T18:21:23Z

Updated

josephschorr · 2024-12-10T19:12:44Z

Rebased

ecordell · 2024-11-04T20:06:40Z

internal/datastore/common/relationships.go

+
+// StaticValueOrAddColumnForSelect adds a column to the list of columns to select if the value
+// is not static, otherwise it sets the value to the static value.
+func StaticValueOrAddColumnForSelect(colsToSelect []any, queryInfo QueryInfo, colName string, field *string) []any {


why are these []any? aren't columns always []string?

They are actually string reference, and it returns an []any because that's what the rows.Scan requires.

This isn't adding the static value to the slice, its adding the reference to the string value for collecting the non-static value

ecordell · 2024-12-10T02:40:16Z

internal/datastore/dsfortesting/dsfortesting.go

+// and injects additional proxies for validation at test time.
+// NOTE: These additional proxies are not performant for use in production (but then,
+// neither is memdb)
+func NewMemDBDatastoreForTesting(


Can we keep this in the memdb package? This just feels hard to find.

I didn't want the memdb package to have requirements on any of the common SQL code, since it is an odd mixture

ecordell · 2024-12-10T02:58:51Z

internal/datastore/common/sql.go

+
+// WithAdditionalFilter returns a new SchemaQueryFilterer with an additional filter applied to the query.
+func (sqf SchemaQueryFilterer) WithAdditionalFilter(filter func(original sq.SelectBuilder) sq.SelectBuilder) SchemaQueryFilterer {
+	return SchemaQueryFilterer{


Instead of this, what about a Clone() method (with a test), and then modify the clone? Then you can't accidentally omit a field in these With methods.

Sure. This is also why I want to have the linter check these, but I'll add a Clone

ecordell · 2024-12-10T21:39:55Z

internal/datastore/common/relationships.go

+
+	colsToSelect = append(colsToSelect, &expiration)
+
+	if withIntegrity {


wouldn't it make sense to have determine this from queryinfo instead of a seperate boolean?

They are injected at different places. To add it to QueryInfo, we'd need a Clone or a WithIntegrity(bool) on QueryInfo. I'm happy to make that change, if you like

ecordell · 2024-12-10T21:58:58Z

internal/datastore/common/relationships.go

+}
+
+// QueryRelationships queries relationships for the given query and transaction.
+func QueryRelationships[R Rows, C ~map[string]any](ctx context.Context, queryInfo QueryInfo, sqlStatement string, args []any, span trace.Span, tx Querier[R], withIntegrity bool) (datastore.RelationshipIterator, error) {


I think it would be simpler to have a QueryRelationshipsNoElision (or whatever name) insted of all of the queryInfo.Schema.ColumnOptimization == ColumnOptimizationOptionNone throughout. Then the query constructors can just pick the implementation on startup and not check at runtime.

This was all intended to be temporary until we end the experiment and remove the flag. I can change it to use a different query function as you suggest, but I'm not sure its worth the effort for a temp flag

ecordell · 2024-12-10T22:14:39Z

internal/datastore/common/sql.go

@@ -580,19 +702,73 @@ func (tqs QueryExecutor) ExecuteQuery(
 		limit = *queryOpts.Limit
 	}

-	toExecute := query.limit(limit)
+	if limit < math.MaxInt64 {
+		query = query.limit(limit)


what's the impact of no longer always having a limit clause?

As far as I can tell, nothing. It just means sending less bytes over the wire

ecordell · 2024-12-10T22:15:32Z

internal/datastore/common/sql.go

+	// Set the column names to select.
+	columnNamesToSelect := make([]string, 0, 8+len(query.extraFields))
+
+	columnNamesToSelect = checkColumn(columnNamesToSelect, query.schema.ColumnOptimization, query.filteringColumnTracker, query.schema.ColNamespace)


this logic is essentially replicating the logic in QueryRelationships - there's no way to unify those two things?

ditto for spanner

Not easily; it is because of Spanner that its hard to do: Spanner needs its own executor code (it can't use the one relationships.go because it has different handling of types for caveats and expiration), but by putting the column selection here, we do share at least that portion

Can column selection be factored out and called from both places? this seems easy to get out of sync.

Yes, we could push it down to the use sites, but it would be fairly similar.

Out of sync is fairly evident: every single read test immediately fails

internal/datastore/common/sql_test.go

ecordell · 2024-12-10T22:40:16Z

internal/datastore/mysql/datastore.go

@@ -417,7 +435,24 @@ type querier interface {
 	QueryContext(context.Context, string, ...interface{}) (*sql.Rows, error)
 }

-func newMySQLExecutor(tx querier) common.ExecuteQueryFunc {
+type wrappedTX struct {


why is it wrapped? can we have a better name here?

It rewrites it to match the interface expected by the common executor. Any suggestions? wrappedForCommon?

yeah anything that will indicate the reason for the wrapping, like withCommonTx or asCommonTx or commonTx

ecordell · 2024-12-11T22:56:00Z

internal/datastore/common/sql.go

 	}

-	toExecute.queryBuilder = toExecute.queryBuilder.From(from)
+	columnNamesToSelect = append(columnNamesToSelect, b.Schema.ColExpiration)


should there be a way to skip expiration fields if we know the rel can't have expiration?

I was going to do it a followup PR, but I'm happy to do so here. Let me know which you prefer

columns that have static values This vastly reduces data over the wire, as well as deserialization time and memory usage

This will allow us to centrally register additional datastore validation that only runs at test time

This validation test acts as a proxy in the memdb testing datastore and validates that the column elision code (which *isn't* used in memdb) matches the static fields to the values returned for all relationships loaded

This moves the behavior out of Spanner datastore and into a common lib where possible

…sabled or the relationship cannot be marked as expiring

Also adds a datastore test to ensure the constructed cursor operates as expected

github-actions bot added area/datastore Affects the storage system area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Oct 21, 2024

josephschorr force-pushed the rel-struct-sql branch 3 times, most recently from d6104da to 44f2cbb Compare October 22, 2024 16:33

josephschorr marked this pull request as ready for review October 22, 2024 16:49

josephschorr requested a review from vroldanbet as a code owner October 22, 2024 16:49

josephschorr requested a review from a team October 22, 2024 16:49

josephschorr changed the title ~~Relationships selected in SQL-based datastores should elide columns that have static values~~ Relationships selected in SQL-based datastores now elide columns that have static values Oct 22, 2024

tstirrat15 previously approved these changes Oct 22, 2024

View reviewed changes

josephschorr dismissed tstirrat15’s stale review via aa6f111 October 23, 2024 18:21

josephschorr force-pushed the rel-struct-sql branch from 44f2cbb to aa6f111 Compare October 23, 2024 18:21

josephschorr force-pushed the rel-struct-sql branch 2 times, most recently from b45f0a7 to 65b4e07 Compare October 28, 2024 15:47

github-actions bot added the area/dispatch Affects dispatching of requests label Oct 28, 2024

josephschorr force-pushed the rel-struct-sql branch 2 times, most recently from aaad075 to 334d8ea Compare October 28, 2024 15:58

josephschorr force-pushed the rel-struct-sql branch from 334d8ea to 228953f Compare November 5, 2024 18:26

github-actions bot added area/CLI Affects the command line area/api v1 Affects the v1 API labels Nov 5, 2024

josephschorr force-pushed the rel-struct-sql branch from 228953f to a904d41 Compare November 6, 2024 17:35

josephschorr force-pushed the rel-struct-sql branch from a904d41 to 75efa5a Compare November 22, 2024 12:18

josephschorr requested a review from a team as a code owner November 22, 2024 18:21

josephschorr force-pushed the rel-struct-sql branch from 96bcf17 to d98d7d2 Compare November 22, 2024 18:27

josephschorr force-pushed the rel-struct-sql branch from d98d7d2 to a833e25 Compare December 10, 2024 19:12

ecordell reviewed Dec 10, 2024

View reviewed changes

josephschorr force-pushed the rel-struct-sql branch from 0d16bd1 to e0692f6 Compare December 11, 2024 19:47

ecordell reviewed Dec 11, 2024

View reviewed changes

josephschorr force-pushed the rel-struct-sql branch 10 times, most recently from 71d25bc to c6dc6d0 Compare December 19, 2024 20:01

josephschorr added 11 commits December 20, 2024 13:29

Relationships selected in SQL-based datastores should elide

7fd2bc0

columns that have static values This vastly reduces data over the wire, as well as deserialization time and memory usage

Skip loading of caveats in SQL when unnecessary

c0458b9

Add additional testing for column elision

14a70c4

Change tests to use a new entrypoint for creating memdb for testing

8e642d4

This will allow us to centrally register additional datastore validation that only runs at test time

Add validation test for elision of columns

c7978d0

This validation test acts as a proxy in the memdb testing datastore and validates that the column elision code (which *isn't* used in memdb) matches the static fields to the values returned for all relationships loaded

Move column elision behind an experimental flag

95af278

Update tests for expiration filtering

6f51b80

Implement a combined builder pattern for relationship SQL construction

582aa05

This moves the behavior out of Spanner datastore and into a common lib where possible

Elide the expiration column and checking when expiration is either di…

8951ce8

…sabled or the relationship cannot be marked as expiring

Add testing for expanded comparison logic in SQL builder

21c3dd3

Also adds a datastore test to ensure the constructed cursor operates as expected

Add additional tracing to relationship querying

0df382c

josephschorr force-pushed the rel-struct-sql branch from 1575c7f to 0df382c Compare December 20, 2024 18:35


		colsToSelect = append(colsToSelect, &expiration)

		if withIntegrity {

Relationships selected in SQL-based datastores now elide columns that have static values #2096

Are you sure you want to change the base?

Relationships selected in SQL-based datastores now elide columns that have static values #2096

Conversation

josephschorr commented Oct 21, 2024 • edited Loading

tstirrat15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephschorr commented Oct 23, 2024

josephschorr commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephschorr commented Oct 21, 2024 •

edited

Loading