fix(prettyCQL): prettyCQL crashes on ouf-of-bound access #433
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
|
CI Failure is now related to #431 and not to the crashing on out of bounds error on |
dkropachev
requested changes
Oct 23, 2024
|
Benchmark results Another benchmark with Really don't know why Iterators optimize the allocations and where they do, but looks like it does, allocations take around 60ns to preform, and this ~500ns gap can be explained by that |
CodeLieutenant
force-pushed
the
fix--prettyCQL
branch
7 times, most recently
from
8c0a31e to
9edf169
Compare
When gemini tries to `pretty print` CQL, in some cases can fail with `out-of-bounds` access on array. ```go out = append(out, queryChunks[qID]) ``` This fix uses new go1.23 iter funcs, which removes the allocations, and is not concerened with indexes which caused the issue in the first place. I tried to remove the `switch .(type)` but `TuppleType` is different so has to be still there. Signed-off-by: Dusan Malusev <[email protected]>
CodeLieutenant
force-pushed
the
fix--prettyCQL
branch
2 times, most recently
from
63f24f2 to
c66cc89
Compare
Not prettyCQL uses the `strings.Builder` to generate the `CQL` statament that was previously executed on ScyllaDB. Instaface has been changed accomadate the `builder` being passed to down. Signed-off-by: Dusan Malusev <[email protected]>
CodeLieutenant
force-pushed
the
fix--prettyCQL
branch
from
c66cc89 to
084f2ca
Compare
dkropachev
requested changes
Oct 30, 2024
everything that can produce a `Pretty CQL` for logging now returns an error, to indicate bad types pass on to it. Signed-off-by: Dusan Malusev <[email protected]>
CodeLieutenant
force-pushed
the
fix--prettyCQL
branch
2 times, most recently
from
2390abf to
8da860f
Compare
Statement Logger had a flow, it assumed it has the owner ship of the query, but statements are reused this means, it produced queries that are not even possible to execute. (This does not fix the issue of more columns expected then returned). Since logger works as single thread to write to the file (Queue), it has to be reworked to format the query and the values before they are passed to the channel (works like a queue), this way a query is owned by the channel inside a bytes.Buffer. PrettyCQL has been rewritten to use `bytes.Buffer` instead of `strings.Builder`, as the intent of the PrettyCQL is to be stored inside a file, and files work with `[]byte` rather then strings. Signed-off-by: Dusan Malusev <[email protected]>
CodeLieutenant
force-pushed
the
fix--prettyCQL
branch
from
8da860f to
4ef2409
Compare
dkropachev
requested changes
Nov 1, 2024
Signed-off-by: Dusan Malusev <[email protected]>
… Integers Signed-off-by: Dusan Malusev <[email protected]>
|
I dont know why, no explanation, but these changes made gemini stable. Further investigation is needed as nothing much has been done to affect how gemini works |
makes sense to me, you switch a few cases from panicking to erroring, it can make a lot of difference |
dkropachev
requested changes
Nov 6, 2024
dkropachev
approved these changes
Nov 12, 2024
dkropachev
reviewed
Nov 12, 2024
Comment on lines
+124
to
+157
| func prettyCQLOld(query string, values Values, types Types) string { | ||
| if len(values) == 0 { | ||
| return query | ||
| } | ||
|
|
||
| k := 0 | ||
| out := make([]string, 0, len(values)*2) | ||
| queryChunks := strings.Split(query, "?") | ||
| out = append(out, queryChunks[0]) | ||
| qID := 1 | ||
| builder := bytes.NewBuffer(nil) | ||
| for _, typ := range types { | ||
| builder.Reset() | ||
| tupleType, ok := typ.(*TupleType) | ||
| if !ok { | ||
| _ = typ.CQLPretty(builder, values[k]) | ||
| out = append(out, builder.String()) | ||
| out = append(out, queryChunks[qID]) | ||
| qID++ | ||
| k++ | ||
| continue | ||
| } | ||
| for _, t := range tupleType.ValueTypes { | ||
| builder.Reset() | ||
| _ = t.CQLPretty(builder, values[k]) | ||
| out = append(out, builder.String()) | ||
| out = append(out, queryChunks[qID]) | ||
| qID++ | ||
| k++ | ||
| } | ||
| } | ||
| out = append(out, queryChunks[qID:]...) | ||
| return strings.Join(out, "") | ||
| } |
There was a problem hiding this comment.
should you drop this ?
Comment on lines
+175
to
+183
| b.Run("Old", func(b *testing.B) { | ||
| b.ReportAllocs() | ||
| b.ResetTimer() | ||
| for i := 0; i < b.N; i++ { | ||
| query, _ := stmt.Query.ToCql() | ||
| values := stmt.Values.Copy() | ||
| prettyCQLOld(query, values, stmt.Types) | ||
| } | ||
| }) |
| return strings.Join(out, "") | ||
| } | ||
|
|
||
| func BenchmarkPrettyCQLOLD(b *testing.B) { |
There was a problem hiding this comment.
Could you please rename it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When gemini tries to
pretty printCQL, in some cases can fail without-of-boundsaccess on array.