go {livesql,sqlgen,internal}: support []byte arguments for live queries #172

berfarah · 2018-09-20T23:56:14Z

Problem

We have UUIDs which use or convert to []byte. Both livesql and the batcher rely on comparable/hashable values (values that are put into a map) to work and panic otherwise. This means that we can't use those custom types with livesql or batching.

Solution

LiveSQL

The ToArray function has been used for creating hashable/comparable arguments in the past. Renamed it to be more explicit, and also handle []byte arguments, converting them into strings.

The primary use-case for this PR is to support UUIDs. We also get support for anything else that turns into []byte.

The reason we want to serialize our arguments/filters is because (unfortunately) we support type aliases in our filters, and therefore allow you to query a int64 with int/int8/etc. In order to maintain backward compatibility there, we have to assume that we're dealing with a SQL value for caching/rerunning.

Batching

TL;DR: do nothing

Batching deals with go struct types (not go sql types), which means we have many possible permutations of types. We could take our filter values and convert them to their sql values, using that to compare. This would take more time than simply using a [16]byte for our UUIDs.

It would properly implement 100% compatibility with custom types. However, I have some concerns about performance in doing so, and given deadline constraints, we can add that on later rather than doing so now.

Slices in filters will continue not to be supported for batching.

coveralls · 2018-09-20T23:59:01Z

Pull Request Test Coverage Report for Build 1088

24 of 24 (100.0%) changed or added relevant lines in 3 files are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.1%) to 65.706%

Files with Coverage Reduction	New Missed Lines	%
graphql/schemabuilder/reflect.go	4	72.07%

Totals
Change from base Build 1084:	0.1%
Covered Lines:	3464
Relevant Lines:	5272

💛 - Coveralls

berfarah · 2018-09-21T00:15:25Z

livesql/integration_test.go

+	users := make(chan *User)
+	rerunner := reactive.NewRerunner(context.Background(), func(ctx context.Context) (interface{}, error) {
+		user := &User{}
+		if err := db.QueryRow(ctx, &user, sqlgen.Filter{"mood": &mood}, nil); err != nil {


This means UUIDs should definitely work

changpingc · 2018-09-21T00:40:56Z

livesql/integration_test.go

+
+	// We use a channel to pass around query updates for testing.
+	users := make(chan *User)
+	rerunner := reactive.NewRerunner(context.Background(), func(ctx context.Context) (interface{}, error) {


try using batch.WithBatching(ctx)

changpingc · 2018-09-21T01:23:57Z

cute idea: check reflect.Type.Comparable()

internal/reflect.go

jellevandenhooff · 2018-09-21T00:27:44Z

internal/reflect.go

 // [...]interface{} for use as a comparable map key
-//
-func ToArray(s []interface{}) interface{} {
+func MakeHashable(s []interface{}) interface{} {


I wonder if we can do the byte to string conversion at a higher level where we create the byte slices

My concern would be breaking things at the SQL layer if I put it higher up. If I were to pick out a refactor that could be worth doing, it's making the filter more responsible for its various conversions so that we don't have to hold in our heads what these things ^ are used for.

Looks like batching doesn't actually use the driver.Value. Need to find a way to do so.

jellevandenhooff · 2018-09-21T00:28:27Z

internal/reflect.go

+	// Convert byte slices into strings as they are otherwise not comparable/hashable.
+	for i, elem := range s {
+		if b, ok := elem.([]byte); ok {
+			s[i] = string(b)


I think this should be a type Alias of string

berfarah · 2018-09-26T21:52:33Z

@stephen I'm ready to merge this as soon as I have a 👍

stephen

could you add a little context in the PR description for what isn't supported? Think I understand the changes being made, but not the context for what we see that's breaking.

stephen · 2018-09-27T18:30:42Z

internal/reflect.go

 	// fast code paths for short arrays:
 	case 0:
 		return [...]interface{}{}
 	case 1:
-		return [...]interface{}{s[0]}
+		return [...]interface{}{d[0]}


super nit: consider changing out s -> d in the arguments instead so that these references stay the same.

(i also checked find all references so this seems fine as is)

stephen · 2018-09-27T18:31:58Z

internal/reflect.go

@@ -31,34 +31,44 @@ func init() {
 	interfaceTyp = reflect.TypeOf(&x).Elem()
 }

-// ToArray converts a []interface{} slice into an equivalent fixed-length array
+// MakeHashable converts a []interface{} slice into an equivalent fixed-length array


nit: consider doing the rename in a separate step to make the semantic change easier to read

## Problem We have UUIDs which use or convert to `[]byte`. Both livesql and the batcher rely on comparable/hashable values (values that are put into a `map`) to work and panic otherwise. This means that we can't use those custom types with livesql or batching. ## Solution ### LiveSQL The `ToArray` function has been used for creating hashable/comparable arguments in the past. Renamed it to be more explicit, and also handle `[]byte` arguments, converting them into strings. The primary use-case for this PR is to support UUIDs. We also get support for anything else that turns into `[]byte`. The reason we want to serialize our arguments/filters is because (unfortunately) we support type aliases in our filters, and therefore allow you to query a `int64` with `int`/`int8`/etc. In order to maintain backward compatibility there, we have to assume that we're dealing with a SQL value for caching/rerunning. ### Batching TL;DR: do nothing Batching deals with go struct types (not go sql types), which means we have many possible permutations of types. We _could_ take our filter values and convert them to their sql values, using that to compare. This would take more time than simply using a `[16]byte` for our UUIDs. It would properly implement 100% compatibility with custom types. However, I have some concerns about performance in doing so, and given deadline constraints, we can add that on later rather than doing so now. **Slices in filters will continue not to be supported for batching.**

This method name makes it clearer what the intention is behind the method, rather than just documenting behavior.

berfarah requested review from changpingc, jellevandenhooff and stephen September 20, 2018 23:56

berfarah force-pushed the bf/support-byteslice-types branch from 8acdee9 to a129e23 Compare September 21, 2018 00:12

berfarah commented Sep 21, 2018

View reviewed changes

changpingc reviewed Sep 21, 2018

View reviewed changes

berfarah force-pushed the bf/support-byteslice-types branch from a129e23 to 74ba38d Compare September 21, 2018 04:08

jellevandenhooff reviewed Sep 21, 2018

View reviewed changes

berfarah force-pushed the bf/support-byteslice-types branch from 74ba38d to fd36c92 Compare September 25, 2018 19:49

berfarah changed the title ~~go {livesql,sqlgen,internal}: support []byte arguments for live queries/batching~~ go {livesql,sqlgen,internal}: support []byte arguments for live queries Sep 25, 2018

berfarah force-pushed the bf/support-byteslice-types branch from fd36c92 to 91e52b8 Compare September 25, 2018 21:27

berfarah force-pushed the bf/support-byteslice-types branch from 91e52b8 to 4cf03cf Compare September 27, 2018 16:48

stephen approved these changes Sep 27, 2018

View reviewed changes

berfarah added 3 commits September 27, 2018 11:43

go internal: rename ToArray to MakeHashable

a755225

This method name makes it clearer what the intention is behind the method, rather than just documenting behavior.

go livesql: add integration test for custom types

0d64f47

berfarah force-pushed the bf/support-byteslice-types branch from 4cf03cf to 0d64f47 Compare September 27, 2018 18:44

berfarah merged commit 205b5f9 into master Sep 27, 2018

berfarah deleted the bf/support-byteslice-types branch September 27, 2018 18:46

berfarah added a commit that referenced this pull request Sep 27, 2018

Add CHANGELOG entry for PR #172

ad8691e

berfarah added a commit that referenced this pull request Sep 27, 2018

Add CHANGELOG entry for PR #172

52fe54e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go {livesql,sqlgen,internal}: support []byte arguments for live queries #172

go {livesql,sqlgen,internal}: support []byte arguments for live queries #172

berfarah commented Sep 20, 2018 •

edited

Loading

coveralls commented Sep 20, 2018 •

edited

Loading

berfarah Sep 21, 2018

changpingc Sep 21, 2018

changpingc commented Sep 21, 2018

jellevandenhooff Sep 21, 2018

berfarah Sep 21, 2018

berfarah Sep 21, 2018

jellevandenhooff Sep 21, 2018

berfarah commented Sep 26, 2018

stephen left a comment

stephen Sep 27, 2018

stephen Sep 27, 2018 •

edited

Loading

stephen Sep 27, 2018

berfarah Sep 27, 2018

go {livesql,sqlgen,internal}: support []byte arguments for live queries #172

go {livesql,sqlgen,internal}: support []byte arguments for live queries #172

Conversation

berfarah commented Sep 20, 2018 • edited Loading

Problem

Solution

LiveSQL

Batching

coveralls commented Sep 20, 2018 • edited Loading

Pull Request Test Coverage Report for Build 1088

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changpingc commented Sep 21, 2018

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

berfarah commented Sep 26, 2018

stephen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephen Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berfarah commented Sep 20, 2018 •

edited

Loading

coveralls commented Sep 20, 2018 •

edited

Loading

stephen Sep 27, 2018 •

edited

Loading