pkg/query: Fix panic when diffing two profiles #4681
Conversation
yomete
changed the title
start and end valuesstart and end values
|
🤖 Meticulous spotted visual differences in 1 of 423 screens tested: view and approve differences detected. Last updated for commit 4d1cce4. This comment will update as new commits are pushed. |
…lues" This reverts commit ac65b0f.
yomete
changed the title
start and end values
pkg/query/columnquery.go
Outdated
| } | ||
|
|
||
| profileMetadata = &pb.ProfileMetadata{ | ||
| MappingFiles: append(mappingFiles_a, mappingFiles_b...), |
There was a problem hiding this comment.
We can’t just append them, we need to merge them. But good news is that they’re both sorted so something like a k-way merge is possible.
pkg/query/columnquery.go
Outdated
| } | ||
|
|
||
| l, err := q.querier.GetProfileMetadataLabels(ctx, req.Match, start, end) | ||
| labels, err := q.GetProfileMetadataLabels(ctx, []string{}, startTime, endTime) |
There was a problem hiding this comment.
We should remove the match variable here. We don’t use it and don’t anticipate using it anytime soon.
pkg/query/columnquery.go
Outdated
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| return mappingFiles, labels, nil | ||
| } | ||
|
|
||
| func KWayMerge(arr1, arr2 []string) []string { |
There was a problem hiding this comment.
this is not 100% right, we need not a pure k-way merge, but it also needs to de-duplicate
I'd say we should just name the function merge and in the code comment say that it does a deduplicating k-way merge, so the two slices that are passed must already be sorted
| @@ -1491,7 +1491,7 @@ func (q *Querier) GetProfileMetadataMappings( | |||
|
|
|||
| values := locations.ListValues().(*array.Dictionary) | |||
|
|
|||
| compactedDict, err := compactDictionary.CompactDictionary(memory.DefaultAllocator, values) | |||
| compactedDict, err := compactDictionary.CompactDictionary(q.pool, values) | |||
There was a problem hiding this comment.
I also made the change to use the Querier's memory allocator here instead of memory.DefaultAllocator.
| return nil, err | ||
| // This is a deduplicating k-way merge. | ||
| // The two slices that are passed in are assumed to be sorted. | ||
| func MergeTwoSortedSlices(arr1, arr2 []string) []string { |
There was a problem hiding this comment.
nit: no need to export this function
There was a problem hiding this comment.
ah I'd exported because the function is being tested in the columnquery.test.go file. is there a better way to do this?
There was a problem hiding this comment.
it's still the same package, within the same package unexported functions can be used
Resolves #4680