CASSGO-130 Add Query.Binding() function to override binding

[max-melentyev]

As Query is supposed to be reusable since cqlbp v2, it should be possible to overwrite a binding too.

[joao-r-reis]

Can you show some examples of the problem you're trying to solve?

[max-melentyev]

Query.Release() was removed in #1868, and the PR description recommends users to re-use Query objects instead. So I assume a code should be changed this way:

// v1
query := session.Query("UPDATE x SET y = ? WHERE z = ?", y1, z1)
query.Exec()
query.Release()
query := session.Query("UPDATE x SET y = ? WHERE z = ?", y2, z2) // Will reuse query object from memory pool
query.Exec()

// v2
query := session.Query("UPDATE x SET y = ? WHERE z = ?", y1, z1)
query.Exec()
query := query.Bind(y2, z2)
query.Exec()

However there's no API for re-using queries created with session.Bind() method. This pr adds this API.

My use-case is to add support for queries with named params and binding values from maps similar to gocqlx. Something like

namedQuery := parse("UPDATE x SET y = :y WHERE z = :z")
query := session.New(namedQuery.CQL())
namedQuery.BindMap(query, map[string]any{"y": y1, "z": z1})

While I can user query.Bind() to set values, i need to set binding to make query fail in the case namedQuery.Bind cannot find necessary arguments:

namedQuery.BindMap(query, map[string]any{"y": y1})
// I'd like to call this inside BindMap:
query.Binding(func(_ QueryInfo) ([]any, error) { return nil, fmt.Errorf("z is missing") })

[joao-r-reis]

The goal of #1868 was mostly to move the execution related data from Query/Batch to Iter so that you can, for example, call .Iter() on the same exact query and parameters multiple times without worrying about metrics, paging data or other similar metadata being overwritten or polluted because the v1 design required you to inspect the Query object post execution to access the metadata related to that execution. I think we still have some work to do to make query objects truly reusable (and safe for concurrent use) by making them immutable for example.

Another goal of that PR was to just get rid of query object pooling altogether because the added complexity is just not worth it (in the rare case that a particular workload does see significant performance impact when objects aren't reused then they can add that pooling/caching on the application side).

But this PR does make sense since we expect users to build their own pooling/caching if they truly want to reuse the query objects even with different parameters and currently you can do that for queries built via Session.Query but not Session.Bind as you correctly point out.

Note that query objects are not safe for concurrent use so if you plan on making some kind of wrapper API then take this into consideration.

[joao-r-reis]

I don't think it's necessary to add this, Session.Bind already uses func(q *QueryInfo) ([]interface{}, error) in its declaration anyway

[max-melentyev]

updated

[max-melentyev]

Note that query objects are not safe for concurrent use so if you plan on making some kind of wrapper API then take this into consideration.

yep. And with previous pooling option it was also possible to re-use queries between different sessions, and with different statement. I think it would be great to remove reference to a Session from Query object. This way it will be possible to cache fully configured query object (retries, timeouts, etc.) and re-use it even if session needs reconnection, for example if credentials needs to be rotated.

[max-melentyev]

I'm not sure if any sort of memory pooling make sense, as internalQuery and queryOptions are instantiated on every request. Caching pre-configured query probably still make sense, as it allows to skip a bunch of builder methods.

Something like this:

query := gocql.NewQuery("SELECT ...").Idempotent()...
iter1 := session.BindValues(query, arg1, arg2, ...).Iter(ctx)
iter2 := session.Bind(query, binding).Iter(ctx)

// Where
(*Session) Bind(Query, args...) BoundQuery
type BoundQuery struct {
  query *Query
  values []any
}
// and it's immutable and only has values to execute it.

[worryg0d]

Indeed, queries are currently created by Session.Bind API is not poolable because the binding func cannot be changed.

I'm +1 on this change

[joao-r-reis]

Can you add an integration test for this use case:

1. call session.Bind
2. execute query
3. reuse the same query object, modify the binding by using this new API
4. execute it again

You can use the TestBoundQueryInfo test as a starting point in cassandra_test.go (and you can put this new test on this file).

Also add this to the changelog and format the commit message according to the guidelines outlined in CONTRIBUTING.md

[worryg0d]

I wrote a simple test locally, and it passes:

func TestStaticQueryInfo_OverrideBindingFunction(t *testing.T) {
	session := createSession(t)
	defer session.Close()

	if err := createTable(session, "CREATE TABLE IF NOT EXISTS gocql_test.static_query_info_override (id int, value text, PRIMARY KEY (id))"); err != nil {
		t.Fatalf("failed to create table with error '%v'", err)
	}

	if err := session.Query("INSERT INTO static_query_info_override (id, value) VALUES (?, ?)", 1, "foo").Exec(); err != nil {
		t.Fatalf("insert into static_query_info_override failed, err '%v'", err)
	}

	if err := session.Query("INSERT INTO static_query_info_override (id, value) VALUES (?, ?)", 2, "bar").Exec(); err != nil {
		t.Fatalf("insert into static_query_info_override failed, err '%v'", err)
	}

	qry := session.Bind("SELECT id, value FROM static_query_info_override WHERE id = ?", func(q *QueryInfo) ([]interface{}, error) {
		values := make([]interface{}, 1)
		values[0] = 1
		return values, nil
	})

	iter := qry.Iter()
	var id int
	var value string
	iter.Scan(&id, &value)
	if err := iter.Close(); err != nil {
		t.Fatalf("query with exposed info failed, err '%v'", err)
	}

	if id != 1 {
		t.Fatalf("Expected id %d, but got %d", 113, id)
	}
	if value != "foo" {
		t.Fatalf("Expected value %s, but got %s", "foo", value)
	}

	qry.Binding(func(q *QueryInfo) ([]interface{}, error) {
		values := make([]interface{}, 1)
		values[0] = 2
		return values, nil
	})

	iter = qry.Iter()
	iter.Scan(&id, &value)
	if err := iter.Close(); err != nil {
		t.Fatalf("query with exposed info failed, err '%v'", err)
	}

	if id != 2 {
		t.Fatalf("Expected id %d, but got %d", 2, id)
	}
	if value != "bar" {
		t.Fatalf("Expected value %s, but got %s", "bar", value)
	}
}