Feature implementation from commits c4c9a1b..e154533 #4
Conversation
Bumps [github.com/docker/distribution](https://github.com/docker/distribution) from 2.8.1+incompatible to 2.8.2+incompatible. - [Release notes](https://github.com/docker/distribution/releases) - [Commits](distribution/distribution@v2.8.1...v2.8.2) --- updated-dependencies: - dependency-name: github.com/docker/distribution dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ust (grafana#827) Bumps [h2](https://github.com/hyperium/h2) from 0.3.14 to 0.3.20. - [Release notes](https://github.com/hyperium/h2/releases) - [Changelog](https://github.com/hyperium/h2/blob/master/CHANGELOG.md) - [Commits](hyperium/h2@v0.3.14...v0.3.20) --- updated-dependencies: - dependency-name: h2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [tokio](https://github.com/tokio-rs/tokio) from 1.21.2 to 1.24.2. - [Release notes](https://github.com/tokio-rs/tokio/releases) - [Commits](https://github.com/tokio-rs/tokio/commits) --- updated-dependencies: - dependency-name: tokio dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
) * Pulll tempo sync * BinaryJoinIterator * Remove unused code * Make use of the BinaryJoinIterator and remove JoinIterator * Correctly log the ReadPages on span * Use load on the atomic page stats * Get tests from tempo * Move map predicate and base it off the min/max This predicate still wouldn't work all that well with a fully SeriesID sorted block (so accress row groups). But let's fix that separately * Fix bug when we iterate left iterator based on a higher right result * Test BinaryJoinIterator thoroughly * Optimize when the Seek destination is only a single row away * Fix tests comments * Validate that page reads are as expected * Fix closing and cancellation A call to Next/Seek after a close should also yield a context cancelled. * Do cancel it in the right place * Improve panic messaging * Add nextSlow and uncomment test
* Add app selector search bar
* Add terminationGracePeriodSeconds helm value * Render manifests * Bump chart version * Update chart README
* Fixes iterator At value re-used * make fmt
![codeowlai[bot]](https://avatars.githubusercontent.com/in/1259765?s=60&v=4){kind=small}
| } | ||
|
|
||
| func (it TreeIterator[T]) At() T { | ||
| return it.Tree.Winner().At() |
There was a problem hiding this comment.
🐛 Correctness Issue
Potential nil pointer dereference in At().
There's no nil check before calling methods on it.Tree.Winner() which could cause a runtime crash if the tree is empty or has no winner.
Current Code (Diff):
- return it.Tree.Winner().At()
+ if winner := it.Tree.Winner(); winner != nil {
+ return winner.At()
+ }
+ var zero T
+ return zero📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| return it.Tree.Winner().At() | |
| if winner := it.Tree.Winner(); winner != nil { | |
| return winner.At() | |
| } | |
| var zero T | |
| return zero |
| } | ||
|
|
||
| func (it *TreeIterator[T]) Err() error { | ||
| return it.Tree.Winner().Err() |
There was a problem hiding this comment.
🐛 Correctness Issue
Potential nil pointer dereference in Err().
There's no nil check before calling methods on it.Tree.Winner() which could cause a runtime crash if the tree is empty or has no winner.
Current Code (Diff):
- return it.Tree.Winner().Err()
+ if winner := it.Tree.Winner(); winner != nil {
+ return winner.Err()
+ }
+ return nil📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| return it.Tree.Winner().Err() | |
| if winner := it.Tree.Winner(); winner != nil { | |
| return winner.Err() | |
| } | |
| return nil |
| } | ||
| } | ||
|
|
||
| func (it TreeIterator[T]) At() T { |
There was a problem hiding this comment.
🐛 Correctness Issue
Inconsistent receiver types.
Method At() uses a value receiver while Err() and Close() use pointer receivers, which can lead to unexpected behavior when used through interfaces.
Current Code (Diff):
- func (it TreeIterator[T]) At() T {
+ func (it *TreeIterator[T]) At() T {📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| func (it TreeIterator[T]) At() T { | |
| func (it *TreeIterator[T]) At() T { |
| func (*functionsHelper) clone(f *profilev1.Function) *profilev1.Function { | ||
| return f | ||
| func (*functionsHelper) clone(f *schemav1.InMemoryFunction) *schemav1.InMemoryFunction { | ||
| return &(*f) |
There was a problem hiding this comment.
🐛 Correctness Issue
Inefficient clone implementation.
The clone implementation return &(*f) dereferences and then re-references the pointer, which doesn't create a true deep copy and could lead to unexpected behavior.
Current Code (Diff):
- return &(*f)
+ return proto.Clone(f).(*schemav1.InMemoryFunction)📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| return &(*f) | |
| return proto.Clone(f).(*schemav1.InMemoryFunction) |
| hdr := (*reflect.SliceHeader)(unsafe.Pointer(&b)) | ||
| hdr.Len = len(s) * int(lineSize) | ||
| hdr.Cap = hdr.Len | ||
| hdr.Data = uintptr(unsafe.Pointer(&s[0])) |
There was a problem hiding this comment.
🐛 Correctness Issue
Unsafe memory manipulation risks system crashes.
The hashLines() function uses unsafe pointer manipulation that assumes struct memory layout is contiguous with no padding, which could lead to unpredictable behavior or crashes across different architectures.
Current Code (Diff):
- var b []byte
- hdr := (*reflect.SliceHeader)(unsafe.Pointer(&b))
- hdr.Len = len(s) * int(lineSize)
- hdr.Cap = hdr.Len
- hdr.Data = uintptr(unsafe.Pointer(&s[0]))
- return maphash.Bytes(mapHashSeed, b)
+ var hash maphash.Hash
+ for _, line := range s {
+ hash.WriteUint32(line.FunctionId)
+ hash.WriteInt64(line.Line)
+ }
+ return hash.Sum64()📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| hdr := (*reflect.SliceHeader)(unsafe.Pointer(&b)) | |
| hdr.Len = len(s) * int(lineSize) | |
| hdr.Cap = hdr.Len | |
| hdr.Data = uintptr(unsafe.Pointer(&s[0])) | |
| var hash maphash.Hash | |
| for _, line := range s { | |
| hash.WriteUint32(line.FunctionId) | |
| hash.WriteInt64(line.Line) | |
| } | |
| return hash.Sum64() |
| ) | ||
|
|
||
| type locationsKey struct { | ||
| MappingId uint64 //nolint | ||
| MappingId uint32 //nolint |
There was a problem hiding this comment.
🐛 Correctness Issue
Type reduction risks uint32 overflow.
Changing MappingId from uint64 to uint32 reduces the maximum possible value from 2^64-1 to 2^32-1, which could cause overflow if existing values exceed uint32 max.
Current Code (Diff):
- MappingId uint32 //nolint
+ MappingId uint64 //nolint📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| MappingId uint32 //nolint | |
| MappingId uint64 //nolint |
| func (*locationsHelper) clone(l *schemav1.InMemoryLocation) *schemav1.InMemoryLocation { | ||
| x := *l | ||
| x.Line = make([]schemav1.InMemoryLine, len(l.Line)) |
There was a problem hiding this comment.
🐛 Correctness Issue
Clone behavior change breaks references.
Clone function now creates a deep copy instead of returning the original object, which will break code that relies on modifications to the clone affecting the original.
Proposed Code:
func (*locationsHelper) clone(l *schemav1.InMemoryLocation) *schemav1.InMemoryLocation {
x := *l
x.Line = make([]schemav1.InMemoryLine, len(l.Line))
copy(x.Line, l.Line)
return &x
}
| func (*mappingsHelper) clone(m *profilev1.Mapping) *profilev1.Mapping { | ||
| return m | ||
| func (*mappingsHelper) clone(m *schemav1.InMemoryMapping) *schemav1.InMemoryMapping { | ||
| return &(*m) |
There was a problem hiding this comment.
🐛 Correctness Issue
Incorrect clone implementation.
The change from 'return m' to 'return &(*m)' doesn't create a clone but returns a pointer to the same data, which could lead to unexpected data mutations.
Current Code (Diff):
- return &(*m)
+ return proto.Clone(m).(*schemav1.InMemoryMapping)📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| return &(*m) | |
| return proto.Clone(m).(*schemav1.InMemoryMapping) |
| func NewMapPredicate[K constraints.Integer, V any](m map[K]V) Predicate { | ||
|
|
||
| var min, max int64 | ||
|
|
||
| first := true | ||
| for k := range m { | ||
| if first || max < int64(k) { | ||
| max = int64(k) | ||
| } | ||
| if first || min > int64(k) { | ||
| min = int64(k) | ||
| } | ||
| first = false | ||
| } | ||
|
|
||
| return &mapPredicate[K, V]{ | ||
| inbetweenPred: NewIntBetweenPredicate(min, max), | ||
| m: m, | ||
| } | ||
| } |
There was a problem hiding this comment.
🐛 Correctness Issue
Empty map causes uninitialized min/max values.
The NewMapPredicate function doesn't handle empty maps, which would result in uninitialized min/max values being used to create an invalid range predicate.
Current Code (Diff):
- func NewMapPredicate[K constraints.Integer, V any](m map[K]V) Predicate {
-
- var min, max int64
-
- first := true
- for k := range m {
- if first || max < int64(k) {
- max = int64(k)
- }
- if first || min > int64(k) {
- min = int64(k)
- }
- first = false
- }
-
- return &mapPredicate[K, V]{
- inbetweenPred: NewIntBetweenPredicate(min, max),
- m: m,
- }
- }
+ func NewMapPredicate[K constraints.Integer, V any](m map[K]V) Predicate {
+ if len(m) == 0 {
+ return &mapPredicate[K, V]{
+ inbetweenPred: NewIntBetweenPredicate(0, -1), // Empty range
+ m: m,
+ }
+ }
+
+ var min, max int64
+
+ first := true
+ for k := range m {
+ if first || max < int64(k) {
+ max = int64(k)
+ }
+ if first || min > int64(k) {
+ min = int64(k)
+ }
+ first = false
+ }
+
+ return &mapPredicate[K, V]{
+ inbetweenPred: NewIntBetweenPredicate(min, max),
+ m: m,
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| func NewMapPredicate[K constraints.Integer, V any](m map[K]V) Predicate { | |
| var min, max int64 | |
| first := true | |
| for k := range m { | |
| if first || max < int64(k) { | |
| max = int64(k) | |
| } | |
| if first || min > int64(k) { | |
| min = int64(k) | |
| } | |
| first = false | |
| } | |
| return &mapPredicate[K, V]{ | |
| inbetweenPred: NewIntBetweenPredicate(min, max), | |
| m: m, | |
| } | |
| } | |
| func NewMapPredicate[K constraints.Integer, V any](m map[K]V) Predicate { | |
| if len(m) == 0 { | |
| return &mapPredicate[K, V]{ | |
| inbetweenPred: NewIntBetweenPredicate(0, -1), // Empty range | |
| m: m, | |
| } | |
| } | |
| var min, max int64 | |
| first := true | |
| for k := range m { | |
| if first || max < int64(k) { | |
| max = int64(k) | |
| } | |
| if first || min > int64(k) { | |
| min = int64(k) | |
| } | |
| first = false | |
| } | |
| return &mapPredicate[K, V]{ | |
| inbetweenPred: NewIntBetweenPredicate(min, max), | |
| m: m, | |
| } | |
| } |
| _, exists := m.m[K(v.Int64())] | ||
| return exists |
There was a problem hiding this comment.
🐛 Correctness Issue
Unsafe type conversion without validation.
The code performs an unchecked type conversion from Int64 to K without validating if the value is of integer type or within K's range, which could cause runtime errors or data corruption.
Current Code (Diff):
- _, exists := m.m[K(v.Int64())]
- return exists
+ if v, ok := v.(pq.Int64Value); ok {
+ intVal := v.Int64()
+ // Check bounds if K is a smaller integer type
+ if intVal >= math.MinInt64 && intVal <= math.MaxInt64 {
+ _, exists := m.m[K(intVal)]
+ return exists
+ }
+ }
+ return false📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| _, exists := m.m[K(v.Int64())] | |
| return exists | |
| if v, ok := v.(pq.Int64Value); ok { | |
| intVal := v.Int64() | |
| // Check bounds if K is a smaller integer type | |
| if intVal >= math.MinInt64 && intVal <= math.MaxInt64 { | |
| _, exists := m.m[K(intVal)] | |
| return exists | |
| } | |
| } | |
| return false |
| @@ -108,16 +108,16 @@ func (b *singleBlockQuerier) resolvePprofSymbols(ctx context.Context, profileSam | |||
| mappingIDs = newUniqueIDs[lo.Tuple2[*profile.Mapping, *googlev1.Mapping]]() | |||
| locations = b.locations.retrieveRows(ctx, locationsIdsByStacktraceID.locationIds().iterator()) | |||
| locationModelsByIds = map[uint64]*profile.Location{} | |||
| functionModelsByIds = map[uint64]*profile.Function{} | |||
| functionModelsByIds = map[uint32]*profile.Function{} | |||
There was a problem hiding this comment.
🐛 Correctness Issue
Data Loss Risk: Map Key Type Reduction.
Changing the map key from uint64 to uint32 can cause data loss or key collisions if function IDs exceed uint32 maximum value (4,294,967,295).
Current Code (Diff):
- functionModelsByIds = map[uint32]*profile.Function{}
+ functionModelsByIds = map[uint64]*profile.Function{}📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| functionModelsByIds = map[uint32]*profile.Function{} | |
| functionModelsByIds = map[uint64]*profile.Function{} |
| @@ -185,8 +185,8 @@ func (b *singleBlockQuerier) resolvePprofSymbols(ctx context.Context, profileSam | |||
| m.A.HasLineNumbers = cur.Result.HasLineNumbers | |||
| m.A.HasInlineFrames = cur.Result.HasInlineFrames | |||
|
|
|||
| stringsIds[cur.Result.Filename] = 0 | |||
| stringsIds[cur.Result.BuildId] = 0 | |||
| stringsIds[int64(cur.Result.Filename)] = 0 | |||
There was a problem hiding this comment.
🐛 Correctness Issue
Type Mismatch: Incorrect Type Casting.
Casting string values to int64 will cause runtime type errors as stringsIds appears to be a map with string keys.
Current Code (Diff):
- stringsIds[int64(cur.Result.Filename)] = 0
+ stringsIds[cur.Result.Filename] = 0📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| stringsIds[int64(cur.Result.Filename)] = 0 | |
| stringsIds[cur.Result.Filename] = 0 |
| stringsIds[cur.Result.Filename] = 0 | ||
| stringsIds[cur.Result.BuildId] = 0 | ||
| stringsIds[int64(cur.Result.Filename)] = 0 | ||
| stringsIds[int64(cur.Result.BuildId)] = 0 |
There was a problem hiding this comment.
🐛 Correctness Issue
Type Mismatch: Incorrect Type Casting.
Casting string values to int64 will cause runtime type errors as stringsIds appears to be a map with string keys.
Current Code (Diff):
- stringsIds[int64(cur.Result.BuildId)] = 0
+ stringsIds[cur.Result.BuildId] = 0📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| stringsIds[int64(cur.Result.BuildId)] = 0 | |
| stringsIds[cur.Result.BuildId] = 0 |
| @@ -290,7 +290,7 @@ func (b *singleBlockQuerier) resolveSymbols(ctx context.Context, stacktracesByMa | |||
| for functions.Next() { | |||
| s := functions.At() | |||
|
|
|||
| functionIDsByStringID[s.Result.Name] = append(functionIDsByStringID[s.Result.Name], s.RowNum) | |||
| functionIDsByStringID[int64(s.Result.Name)] = append(functionIDsByStringID[int64(s.Result.Name)], s.RowNum) | |||
There was a problem hiding this comment.
🐛 Correctness Issue
Type mismatch in map key.
Converting s.Result.Name to int64 could cause runtime panics if the original map is defined with string keys or if the value isn't convertible to int64.
Current Code (Diff):
- functionIDsByStringID[int64(s.Result.Name)] = append(functionIDsByStringID[int64(s.Result.Name)], s.RowNum)
+ functionIDsByStringID[s.Result.Name] = append(functionIDsByStringID[s.Result.Name], s.RowNum)📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| functionIDsByStringID[int64(s.Result.Name)] = append(functionIDsByStringID[int64(s.Result.Name)], s.RowNum) | |
| functionIDsByStringID[s.Result.Name] = append(functionIDsByStringID[s.Result.Name], s.RowNum) |
| @@ -175,8 +175,8 @@ func (b *singleBlockQuerier) resolvePprofSymbols(ctx context.Context, profileSam | |||
| for mapping.Next() { | |||
| cur := mapping.At() | |||
| m := mappingIDs[int64(cur.Result.Id)] | |||
| m.B.Filename = cur.Result.Filename | |||
| m.B.BuildId = cur.Result.BuildId | |||
| m.B.Filename = int64(cur.Result.Filename) | |||
There was a problem hiding this comment.
🐛 Correctness Issue
Potential type mismatch with int64 conversion.
Converting what appears to be string values (Filename and BuildId) to int64 could cause runtime exceptions if these fields are meant to store strings.
Current Code (Diff):
- m.B.Filename = int64(cur.Result.Filename)
+ m.B.Filename = cur.Result.Filename📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| m.B.Filename = int64(cur.Result.Filename) | |
| m.B.Filename = cur.Result.Filename |
| m.B.Filename = cur.Result.Filename | ||
| m.B.BuildId = cur.Result.BuildId | ||
| m.B.Filename = int64(cur.Result.Filename) | ||
| m.B.BuildId = int64(cur.Result.BuildId) |
There was a problem hiding this comment.
🐛 Correctness Issue
Potential type mismatch with int64 conversion.
Converting what appears to be string values (BuildId) to int64 could cause runtime exceptions if this field is meant to store strings.
Current Code (Diff):
- m.B.BuildId = int64(cur.Result.BuildId)
+ m.B.BuildId = cur.Result.BuildId📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| m.B.BuildId = int64(cur.Result.BuildId) | |
| m.B.BuildId = cur.Result.BuildId |
| SystemName: s.Result.SystemName, | ||
| Filename: s.Result.Filename, | ||
| StartLine: s.Result.StartLine, | ||
| Name: int64(s.Result.Name), |
There was a problem hiding this comment.
🐛 Correctness Issue
Type mismatch: Converting string to int64.
Converting string fields to int64 will likely cause runtime errors as the googlev1.Function struct probably expects these fields to be strings.
Current Code (Diff):
- Name: int64(s.Result.Name),
+ Name: s.Result.Name,📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| Name: int64(s.Result.Name), | |
| Name: s.Result.Name, |
| Filename: s.Result.Filename, | ||
| StartLine: s.Result.StartLine, | ||
| Name: int64(s.Result.Name), | ||
| SystemName: int64(s.Result.SystemName), |
There was a problem hiding this comment.
🐛 Correctness Issue
Type mismatch: Converting string to int64.
Converting SystemName from string to int64 will likely cause runtime errors as the googlev1.Function struct probably expects this field to be a string.
Current Code (Diff):
- SystemName: int64(s.Result.SystemName),
+ SystemName: s.Result.SystemName,📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| SystemName: int64(s.Result.SystemName), | |
| SystemName: s.Result.SystemName, |
| StartLine: s.Result.StartLine, | ||
| Name: int64(s.Result.Name), | ||
| SystemName: int64(s.Result.SystemName), | ||
| Filename: int64(s.Result.Filename), |
There was a problem hiding this comment.
🐛 Correctness Issue
Type mismatch: Converting string to int64.
Converting Filename from string to int64 will likely cause runtime errors as the googlev1.Function struct probably expects this field to be a string.
Current Code (Diff):
- Filename: int64(s.Result.Filename),
+ Filename: s.Result.Filename,📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| Filename: int64(s.Result.Filename), | |
| Filename: s.Result.Filename, |
| Name: int64(s.Result.Name), | ||
| SystemName: int64(s.Result.SystemName), | ||
| Filename: int64(s.Result.Filename), | ||
| StartLine: int64(s.Result.StartLine), |
There was a problem hiding this comment.
🐛 Correctness Issue
Type mismatch: Converting integer to int64.
Converting StartLine from integer to int64 will likely cause runtime errors as the googlev1.Function struct probably expects this field to be an integer.
Current Code (Diff):
- StartLine: int64(s.Result.StartLine),
+ StartLine: s.Result.StartLine,📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| StartLine: int64(s.Result.StartLine), | |
| StartLine: s.Result.StartLine, |
| stringsIds[s.Result.Name] = 0 | ||
| stringsIds[s.Result.Filename] = 0 | ||
| stringsIds[s.Result.SystemName] = 0 | ||
| stringsIds[int64(s.Result.Name)] = 0 |
There was a problem hiding this comment.
🐛 Correctness Issue
Type mismatch: Using int64 as map key.
Using int64 as a map key where a string is expected will cause runtime errors when accessing the stringsIds map.
Current Code (Diff):
- stringsIds[int64(s.Result.Name)] = 0
+ stringsIds[s.Result.Name] = 0📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| stringsIds[int64(s.Result.Name)] = 0 | |
| stringsIds[s.Result.Name] = 0 |
| stringsIds[s.Result.Filename] = 0 | ||
| stringsIds[s.Result.SystemName] = 0 | ||
| stringsIds[int64(s.Result.Name)] = 0 | ||
| stringsIds[int64(s.Result.Filename)] = 0 |
There was a problem hiding this comment.
🐛 Correctness Issue
Type mismatch: Using int64 as map key.
Using int64 as a map key where a string is expected will cause runtime errors when accessing the stringsIds map.
Current Code (Diff):
- stringsIds[int64(s.Result.Filename)] = 0
+ stringsIds[s.Result.Filename] = 0📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| stringsIds[int64(s.Result.Filename)] = 0 | |
| stringsIds[s.Result.Filename] = 0 |
| stringsIds[s.Result.SystemName] = 0 | ||
| stringsIds[int64(s.Result.Name)] = 0 | ||
| stringsIds[int64(s.Result.Filename)] = 0 | ||
| stringsIds[int64(s.Result.SystemName)] = 0 |
There was a problem hiding this comment.
🐛 Correctness Issue
Type mismatch: Using int64 as map key.
Using int64 as a map key where a string is expected will cause runtime errors when accessing the stringsIds map.
Current Code (Diff):
- stringsIds[int64(s.Result.SystemName)] = 0
+ stringsIds[s.Result.SystemName] = 0📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| stringsIds[int64(s.Result.SystemName)] = 0 | |
| stringsIds[s.Result.SystemName] = 0 |
| parquet.Ascending("FileName"), | ||
| ) | ||
| } | ||
| func (*FunctionPersister) SortingColumns() parquet.SortingOption { return parquet.SortingColumns() } |
There was a problem hiding this comment.
🐛 Correctness Issue
Removal of sorting columns breaks query performance.
Removing the sorting columns configuration without replacement could break query performance or result ordering that depends on this sorting.
Current Code (Diff):
- func (*FunctionPersister) SortingColumns() parquet.SortingOption { return parquet.SortingColumns() }
+ func (*FunctionPersister) SortingColumns() parquet.SortingOption { return parquet.SortingColumns(
+ parquet.Ascending("Id"),
+ parquet.Ascending("Name"),
+ parquet.Ascending("Filename"),
+ ) }📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| func (*FunctionPersister) SortingColumns() parquet.SortingOption { return parquet.SortingColumns() } | |
| func (*FunctionPersister) SortingColumns() parquet.SortingOption { return parquet.SortingColumns( | |
| parquet.Ascending("Id"), | |
| parquet.Ascending("Name"), | |
| parquet.Ascending("Filename"), | |
| ) } |
| @@ -6,35 +6,50 @@ import ( | |||
| profilev1 "github.com/grafana/phlare/api/gen/proto/go/google/v1" | |||
| ) | |||
|
|
|||
| var functionsSchema = parquet.SchemaOf(&profilev1.Function{}) | |||
| var functionsSchema = parquet.SchemaOf(new(profilev1.Function)) | |||
There was a problem hiding this comment.
🐛 Correctness Issue
Schema-implementation type mismatch.
The schema definition still uses profilev1.Function but implementation now uses InMemoryFunction, creating an inconsistency that will cause runtime errors.
Current Code (Diff):
- var functionsSchema = parquet.SchemaOf(new(profilev1.Function))
+ var functionsSchema = parquet.SchemaOf(new(InMemoryFunction))📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| var functionsSchema = parquet.SchemaOf(new(profilev1.Function)) | |
| var functionsSchema = parquet.SchemaOf(new(InMemoryFunction)) |
|
|
||
| func (*LocationPersister) Schema() *parquet.Schema { return locationsSchema } | ||
|
|
||
| func (*LocationPersister) SortingColumns() parquet.SortingOption { return parquet.SortingColumns() } |
There was a problem hiding this comment.
⚡ Performance Issue
Critical Performance: Sorting Columns Removed.
The SortingColumns() implementation now returns an empty configuration, removing previous sorting by Id, MappingId, and Address, which could severely impact query performance.
Current Code (Diff):
- func (*LocationPersister) SortingColumns() parquet.SortingOption { return parquet.SortingColumns() }
+ func (*LocationPersister) SortingColumns() parquet.SortingOption { return parquet.SortingColumns(
+ parquet.Ascending("Id"),
+ parquet.Ascending("MappingId"),
+ parquet.Ascending("Address"),
+ )}📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| func (*LocationPersister) SortingColumns() parquet.SortingOption { return parquet.SortingColumns() } | |
| func (*LocationPersister) SortingColumns() parquet.SortingOption { return parquet.SortingColumns( | |
| parquet.Ascending("Id"), | |
| parquet.Ascending("MappingId"), | |
| parquet.Ascending("Address"), | |
| )} |
| } | ||
| return 0, &location, nil | ||
| return 0, &loc, nil |
There was a problem hiding this comment.
🐛 Correctness Issue
Critical Correctness: ID Always Returns 0.
The Reconstruct method always returns 0 as the ID despite reading loc.Id from row[0], creating data inconsistency that could lead to runtime errors.
Current Code (Diff):
- return 0, &loc, nil
+ return loc.Id, &loc, nil📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| return 0, &loc, nil | |
| return loc.Id, &loc, nil |
| } | ||
| row = row[:0] | ||
| row = append(row, parquet.Int64Value(int64(loc.Id)).Level(0, 0, newCol())) | ||
| row = append(row, parquet.Int32Value(int32(loc.MappingId)).Level(0, 0, newCol())) |
There was a problem hiding this comment.
🐛 Correctness Issue
Data Truncation Risk: uint32/int32 Conversion.
Converting MappingId from uint64 to int32 could cause data truncation if values exceed the int32 range, leading to incorrect data.
Current Code (Diff):
- row = append(row, parquet.Int32Value(int32(loc.MappingId)).Level(0, 0, newCol()))
+ row = append(row, parquet.Int64Value(int64(loc.MappingId)).Level(0, 0, newCol()))📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| row = append(row, parquet.Int32Value(int32(loc.MappingId)).Level(0, 0, newCol())) | |
| row = append(row, parquet.Int64Value(int64(loc.MappingId)).Level(0, 0, newCol())) |
| var mapping profilev1.Mapping | ||
| if err := mappingsSchema.Reconstruct(&mapping, row); err != nil { | ||
| return 0, nil, err | ||
| func (*MappingPersister) Reconstruct(row parquet.Row) (uint64, *InMemoryMapping, error) { |
There was a problem hiding this comment.
🐛 Correctness Issue
Missing Index Range Validation.
The Reconstruct method accesses row indices without bounds checking, which will cause runtime panics if row length < 10.
Current Code (Diff):
- func (*MappingPersister) Reconstruct(row parquet.Row) (uint64, *InMemoryMapping, error) {
- mapping := InMemoryMapping{
- Id: row[0].Uint64(),
- MemoryStart: row[1].Uint64(),
- MemoryLimit: row[2].Uint64(),
- FileOffset: row[3].Uint64(),
- Filename: row[4].Uint32(),
- BuildId: row[5].Uint32(),
- HasFunctions: row[6].Boolean(),
- HasFilenames: row[7].Boolean(),
- HasLineNumbers: row[8].Boolean(),
- HasInlineFrames: row[9].Boolean(),
- }
- return 0, &mapping, nil
+ func (*MappingPersister) Reconstruct(row parquet.Row) (uint64, *InMemoryMapping, error) {
+ if len(row) < 10 {
+ return 0, nil, fmt.Errorf("expected row with at least 10 columns, got %d", len(row))
+ }
+ mapping := InMemoryMapping{
+ Id: row[0].Uint64(),
+ MemoryStart: row[1].Uint64(),
+ MemoryLimit: row[2].Uint64(),
+ FileOffset: row[3].Uint64(),
+ Filename: row[4].Uint32(),
+ BuildId: row[5].Uint32(),
+ HasFunctions: row[6].Boolean(),
+ HasFilenames: row[7].Boolean(),
+ HasLineNumbers: row[8].Boolean(),
+ HasInlineFrames: row[9].Boolean(),
+ }
+ return mapping.Id, &mapping, nil📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| func (*MappingPersister) Reconstruct(row parquet.Row) (uint64, *InMemoryMapping, error) { | |
| func (*MappingPersister) Reconstruct(row parquet.Row) (uint64, *InMemoryMapping, error) { | |
| if len(row) < 10 { | |
| return 0, nil, fmt.Errorf("expected row with at least 10 columns, got %d", len(row)) | |
| } | |
| mapping := InMemoryMapping{ | |
| Id: row[0].Uint64(), | |
| MemoryStart: row[1].Uint64(), | |
| MemoryLimit: row[2].Uint64(), | |
| FileOffset: row[3].Uint64(), | |
| Filename: row[4].Uint32(), | |
| BuildId: row[5].Uint32(), | |
| HasFunctions: row[6].Boolean(), | |
| HasFilenames: row[7].Boolean(), | |
| HasLineNumbers: row[8].Boolean(), | |
| HasInlineFrames: row[9].Boolean(), | |
| } | |
| return mapping.Id, &mapping, nil |
| HasFunctions: row[6].Boolean(), | ||
| HasFilenames: row[7].Boolean(), | ||
| HasLineNumbers: row[8].Boolean(), | ||
| HasInlineFrames: row[9].Boolean(), | ||
| } | ||
| return 0, &mapping, nil |
There was a problem hiding this comment.
🐛 Correctness Issue
Incorrect ID Return Value.
The Reconstruct method always returns 0 for the ID instead of the mapping's actual ID, which is inconsistent with the return signature.
Current Code (Diff):
- return 0, &mapping, nil
+ return mapping.Id, &mapping, nil📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| return 0, &mapping, nil | |
| return mapping.Id, &mapping, nil |
PR Summary
Refactor Iterator Implementation with Improved Error Handling
Overview
This PR refactors the iterator implementation across multiple packages with a focus on robust error handling, performance optimization, and code clarity. It introduces specialized iterator types (TreeIterator, SyncIterator, BinaryJoinIterator) while standardizing error handling patterns throughout the codebase.
Change Types
Affected Modules
iter/*.goparquet/*.gophlaredb/query/*.gophlaredb/*.gophlaredb/schemas/v1/*.goutil/loser/tree.goquerier/select_merge.gosymdb/*.goNotes for Reviewers