Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

using of gocql.UnsetValue #202

Closed
N1cOs opened this issue Nov 16, 2021 · 8 comments · Fixed by #206
Closed

using of gocql.UnsetValue #202

N1cOs opened this issue Nov 16, 2021 · 8 comments · Fixed by #206

Comments

@N1cOs
Copy link
Contributor

N1cOs commented Nov 16, 2021

Hi everyone! Recently we faced a following problem. In one of our processes we create a query by specifying ALL columns of a model struct and bind its fields by using method BindStruct. Some of the struct fields can be nil values and it leads to the problem that we generate ton of tombstones by explicitly sending null values to Cassandra. Base driver gocql has gocql.UnsetValue for unsetting value in statements that are already prepared. Unfortunately, creating custom type and adding method gocql.MarshalCQL doesn't solve the problem because gocql has different logic for set and unset values: we have to pass gocql.UnsetValue directly into query.Bind method.

To solve this problem we have some suggestions:

  • Add method like NilAsUnset for Queryx struct. It sets some flag and then method bindStructArgs binds gocql.UnsetValue for all nil fields.
  • Add interface with one method BindCQL() interface{}. Then in binding process check if value implements this interface. For these fields, instead of themselves, bind a return value of BindCQL method. As a result clients have flexibility in defining situations when custom fields should be considered as unset.

What do you think guys? Can we workaround stated problem without adding new functionality? If it's impossible, then we're ready to implement any of the suggested methods by ourselves.

@mmatczuk
Copy link
Contributor

Overall the proposal makes sense.

Ad 1.
This can be added on iterator level if needed similar to Unsafe.

Ad 2.
I'd rather add that as a new option in the db tag.

@N1cOs
Copy link
Contributor Author

N1cOs commented Nov 16, 2021

@mmatczuk

  1. Actually I don't get the proposal about adding it on iterator level. How can we do it for execute queries? Can you please give here a small example?
  2. What kind of option it can be? Like omitempty in json package?

@mmatczuk
Copy link
Contributor

Correct and correct.

@N1cOs
Copy link
Contributor Author

N1cOs commented Nov 17, 2021

Ok, so I have the following plan:

  • I'll rewrite function bindStructArgs. Apparently I need to unwrap function mapper.TraversalsByNameFunc, cause I need options from FieldInfo struct.
  • During traversal I'll check if value is empty and field has tag unsetempty, then I'll bind gocql.UnsetValue
  • Add subtest for empty values in TestQueryxBindStruct function
  • Add some examples

@mmatczuk is it ok?

@mmatczuk
Copy link
Contributor

I want to avoid taxing the main flow with more reflection and keep it simple in general.

I had a look in the code. We could a decorator in Query, if the decorator is set call the decorator on bind.

type Queryx struct {
...
	Transformer func(name string, val interface{}) interface{}

then bindStructArgs goes to Queryx and if Transformer is not nil it's called before adding to args list.

On top of that you can build the UnsetValue transformer.

@N1cOs
Copy link
Contributor Author

N1cOs commented Nov 17, 2021

I see your point but for me is not clear how to get info about tags in such transformer method. If we discard idea about using tags, then I understand how to do it. To be more json-like and use tags we can implement the following transformer method:

type tranformerFn func(fi *reflectx.FieldInfo, val interface{}) interface{}

And then rewrite bindStructArgs method:

tm := m.TypeMap(v.Type())
for i, name := range names {
	fi, ok := tm.Names[name]
	if ok {
		val := reflectx.FieldByIndexesReadOnly(v, fi.Index).Interface()
		if tf != nil { // transformerFn
			val = tf(fi, val)
		}
		arglist = append(arglist, val)
	}
}

As you can see I made tranformerFn as private, because it's low-level for outside clients.

@mmatczuk
Copy link
Contributor

The point is you do not need the tags.
The name and value in the context of a query is enough - the implementation will be simpler.

@N1cOs
Copy link
Contributor Author

N1cOs commented Nov 18, 2021

Ok, got it. I will make a pr in several days

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants