Skip to content
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 for v1.3.0 sqlx.In #688

Closed
johejo opened this issue Jan 25, 2021 · 2 comments
Closed

Changes for v1.3.0 sqlx.In #688

johejo opened this issue Jan 25, 2021 · 2 comments

Comments

@johejo
Copy link

johejo commented Jan 25, 2021

In v1.3.0, sqlx.In returns slice of pointer.

package sqlx_test

import (
	"testing"

	"github.com/jmoiron/sqlx"
)

func TestIn_130(t *testing.T) {
	t.Run("[]interface{}{}", func(t *testing.T) {
		q, args, err := sqlx.In("SELECT * FROM people WHERE name IN (?)", []interface{}{[]string{"gopher"}}...)
		if err != nil {
			t.Fatal(err)
		}
		if q != "SELECT * FROM people WHERE name IN (?)" {
			t.Errorf("got=%v", q)
		}
		t.Log(args)
		for _, a := range args {
			switch a.(type) {
			case string:
				t.Log("ok: string", a)
			case *string:
				t.Error("ng: string pointer", a, *a.(*string))
			}
		}
	})

	t.Run("[]string{}", func(t *testing.T) {
		q, args, err := sqlx.In("SELECT * FROM people WHERE name IN (?)", []string{"gopher"})
		if err != nil {
			t.Fatal(err)
		}
		if q != "SELECT * FROM people WHERE name IN (?)" {
			t.Errorf("got=%v", q)
		}
		t.Log(args)
		for _, a := range args {
			switch a.(type) {
			case string:
				t.Log("ok: string", a)
			case *string:
				t.Error("ng: string pointer", a, *a.(*string))
			}
		}
	})
}

This test passes in v1.2.0 but fails in v1.3.0.

related #635

@jmoiron
Copy link
Owner

jmoiron commented Jan 25, 2021

Thanks, hyrum's law strikes again, I'll add this as a test and see if I need to back that optimization change out.

@njern
Copy link

njern commented Jan 26, 2021

@jmoiron are you planning to cut a 1.3.1 release with this fix included?

ebenoist pushed a commit to reverbdotcom/sqlx that referenced this issue May 11, 2021
The changes in jmoiron#635 changed the some of the output types of In to
pointers.  This takes less time but it also changed the types in the
output of In in a way that I think is more aggressive than I would have
preferred.  I'm also not 100% convinced that using pointers to types like
`int` and `string` would provide an overall performance benefit when you
factor in GC.

Despite that, timings did get worse:

pre-change:

```
BenchmarkIn-4                3136129               376 ns/op             272
B/op           4 allocs/op
BenchmarkIn1k-4                   171588              6602 ns/op
19488 B/op             3 allocs/op
BenchmarkIn1kInt-4                157549              7502 ns/op
19488 B/op             3 allocs/op
BenchmarkIn1kString-4             155502              7604 ns/op
19488 B/op             3 allocs/op
```

post-change:

```
BenchmarkIn-4                    3007132               396 ns/op
272 B/op               4 allocs/op
BenchmarkIn1k-4                   175978              6768 ns/op
19488 B/op             3 allocs/op
BenchmarkIn1kInt-4                120422             10125 ns/op
19488 B/op             3 allocs/op
BenchmarkIn1kString-4             108813             10755 ns/op
19488 B/op             3 allocs/op
```

I'd prefer to keep `[]int{..}` producing ints instead of `*int` even if
it means losing ~25% of perf on these special cased functions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants