[pkg/ottl] Support dynamic indexing #36719
Conversation
github-actions
bot
requested review from
bogdandrutu,
evan-bradley,
kentquirk and
TylerHelmuth
odubajDT
force-pushed
the
indexing-pkg-ottl
branch
5 times, most recently
from
1577fcb to
87f2d33
Compare
odubajDT
force-pushed
the
indexing-pkg-ottl
branch
from
8f9ce11 to
cfc0282
Compare
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
odubajDT
force-pushed
the
indexing-pkg-ottl
branch
from
d39f561 to
5e92cc6
Compare
There was a problem hiding this comment.
Thanks for taking this one on @odubajDT. I have a few questions on the implementation, but based on the tests passing, I think we're definitely on the right track.
| } | ||
|
|
||
| var _ Key[any] = &baseKey[any]{} | ||
|
|
||
| type baseKey[K any] struct { | ||
| s *string | ||
| i *int64 | ||
| p Getter[K] |
There was a problem hiding this comment.
This is minor, but could we make the field name g or something more representative of its type?
| @@ -8,12 +8,12 @@ import ( | |||
| "context" | |||
| "encoding/binary" | |||
| "encoding/hex" | |||
| "encoding/json" | |||
There was a problem hiding this comment.
Could we revert this change? I think your editor may have done this automatically.
| @@ -46,6 +46,7 @@ var _ ottl.Key[any] = &TestKey[any]{} | |||
| type TestKey[K any] struct { | |||
| S *string | |||
| I *int64 | |||
| P ottl.Getter[K] | |||
There was a problem hiding this comment.
| P ottl.Getter[K] | |
| G ottl.Getter[K] |
Same here, I think a G for Getter makes the most sense.
| } | ||
| resVal, ok := res.(T) | ||
| if !ok { | ||
| return nil, fmt.Errorf("casting not successful") |
There was a problem hiding this comment.
| return nil, fmt.Errorf("casting not successful") | |
| return nil, fmt.Errorf("could not resolve key for map/slice, expecting `int64` or `string` but got %T", res) |
It would be nice to make this more explicit what's happening here, if a user ever hits this error we should try to give them all the relevant information we have. If possible, it would also be nice to print the type of T here so we can make it clear what type of key was expected.
| return nil, fmt.Errorf("non-integer indexing is not supported") | ||
| resInt, err := FetchValueFromExpression[K, int64](ctx, tCtx, keys[0]) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("non-integer indexing is not supported") |
There was a problem hiding this comment.
| return nil, fmt.Errorf("non-integer indexing is not supported") | |
| return nil, fmt.Errorf("unable to resolve an integer index: %w", err) |
I think at a minimum we should wrap the underlying error, but we should also consider rewording this message, since I'm not sure a failure in FetchValueFromExpression necessarily means the user tried to use a non-integer value to index a slice. For example, they could have tried to retrieve attributes["my_int"] and the attribute was nil.
| @@ -71,27 +71,35 @@ func SetValue(value pcommon.Value, val any) error { | |||
| func getIndexableValue[K any](ctx context.Context, tCtx K, value pcommon.Value, keys []ottl.Key[K]) (any, error) { | |||
| val := value | |||
| var ok bool | |||
| for i := 0; i < len(keys); i++ { | |||
| for count := 0; count < len(keys); count++ { | |||
There was a problem hiding this comment.
What does count represent here? To me the conventional i makes more sense, but I could be missing your intent. count strikes me as more of an accumulated/computed value.
| resString, errString := FetchValueFromExpression[K, string](ctx, tCtx, keys[count]) | ||
| resInt, errInt := FetchValueFromExpression[K, int64](ctx, tCtx, keys[count]) |
There was a problem hiding this comment.
Would it be possible to take these out of this default case and consider all four cases inside a single switch statement?
| if f := keys[i].Expression.Float; f != nil { | ||
| par = literal[K]{value: *f} | ||
| } | ||
| if i := keys[i].Expression.Int; i != nil { | ||
| par = literal[K]{value: *i} | ||
| } |
There was a problem hiding this comment.
When will these two cases be hit? Would it simplify things if we refactored the grammar to just have keys be a mathExprLiteral type instead of a key type?
Description
Link to tracking issue
Fixes #36644