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

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

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

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 review from vroldanbet and a team as code owners 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 added 3 commits November 5, 2024 12:07

Relationships selected in SQL-based datastores should elide

41519e4

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

e60fec4

Add additional testing for column elision

11031c7

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 added 2 commits November 6, 2024 12:35

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

23edcef

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

Add validation test for elision of columns

a904d41

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

josephschorr commented Oct 21, 2024 •

edited

Loading

tstirrat15 left a comment

tstirrat15 Oct 22, 2024

josephschorr Oct 23, 2024

jzelinskie Nov 5, 2024

josephschorr Nov 5, 2024

tstirrat15 Oct 22, 2024

tstirrat15 Oct 22, 2024

josephschorr Oct 23, 2024

tstirrat15 Oct 22, 2024

tstirrat15 Oct 22, 2024

josephschorr Oct 23, 2024

tstirrat15 Oct 22, 2024

josephschorr Oct 23, 2024

tstirrat15 Oct 22, 2024

josephschorr Oct 23, 2024

tstirrat15 Oct 22, 2024

josephschorr Oct 23, 2024

josephschorr commented Oct 23, 2024

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 Oct 21, 2024 •

edited

Loading