This repository has been archived by the owner on Sep 9, 2024. It is now read-only.
Add ability to have sentinel values for Clustering Columns #71
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ing queries This allows a pre-mediated value to stand in when generating queries for systems (like Keyspaces) that don't support having empty clustering columns
kjgorman
previously approved these changes
Sep 9, 2021
| switch { | ||
| case val.Kind() == reflect.String: | ||
| if val.String() == ClusteringSentinel { | ||
| return true, reflect.New(val.Type()).Elem().Interface() |
There was a problem hiding this comment.
is there a scenario where reflect.New(val.Type()).Elem().Interface() will not just be "" here? could we just return that?
There was a problem hiding this comment.
I initially had that but we need to consider if you have a type alias for which that doesn't work
| } | ||
|
|
||
| elem := reflect.ValueOf(ptr).Elem() | ||
| if isSentinel, nonSentinelValue := isClusteringSentinelValue(elem.Interface()); isSentinel { |
There was a problem hiding this comment.
to what degree (if any) should we be thinking about potential performance regressions by adding an extra step of reflection/value comparison for every field pointer now, do you think?
There was a problem hiding this comment.
We can benchmark this using the existing benchmarks
Before:
goos: darwin
goarch: amd64
pkg: github.com/monzo/gocassa
cpu: Intel(R) Core(TM) i7-8559U CPU @ 2.70GHz
BenchmarkDecodeBlogSliceNoBody
BenchmarkDecodeBlogSliceNoBody-8 29456 34640 ns/op
BenchmarkDecodeBlogStruct
BenchmarkDecodeBlogStruct-8 578539 1841 ns/op
BenchmarkDecodeBlogStructEmptyBody
BenchmarkDecodeBlogStructEmptyBody-8 649267 1840 ns/op
BenchmarkDecodeAlphaSlice
BenchmarkDecodeAlphaSlice-8 10000 101197 ns/op
BenchmarkDecodeAlphaStruct
BenchmarkDecodeAlphaStruct-8 236494 4842 ns/op
BenchmarkStatementMapTable
BenchmarkStatementMapTable-8 412954 2773 ns/op 3664 B/op 33 allocs/op
PASS
ok github.com/monzo/gocassa 7.574s
After:
goos: darwin
goarch: amd64
pkg: github.com/monzo/gocassa
cpu: Intel(R) Core(TM) i7-8559U CPU @ 2.70GHz
BenchmarkDecodeBlogSliceNoBody
BenchmarkDecodeBlogSliceNoBody-8 31040 39350 ns/op
BenchmarkDecodeBlogStruct
BenchmarkDecodeBlogStruct-8 538323 2429 ns/op
BenchmarkDecodeBlogStructEmptyBody
BenchmarkDecodeBlogStructEmptyBody-8 524773 2916 ns/op
BenchmarkDecodeAlphaSlice
BenchmarkDecodeAlphaSlice-8 8732 133401 ns/op
BenchmarkDecodeAlphaStruct
BenchmarkDecodeAlphaStruct-8 181543 6072 ns/op
BenchmarkStatementMapTable
BenchmarkStatementMapTable-8 432660 2886 ns/op 3728 B/op 33 allocs/op
PASS
ok github.com/monzo/gocassa 9.648s
I think we can probably optimise this codepath a bit more (the before is quite optimised since #68)
leesio
approved these changes
Sep 20, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In services like Keyspaces, it is not permitted to have empty string/blob clustering values. This PR introduces a sentinel value that can stand-in in these cases because applications may wish to have empty values (since Cassandra does allow you to do so)