[pkg/ottl]: Add Sort converter #34283
Conversation
kaisecheng
requested review from
TylerHelmuth,
bogdandrutu and
evan-bradley
as code owners
pkg/ottl/ottlfuncs/README.md
Outdated
|
|
||
| `target` is a `pcommon.Slice` type field. `order` is a string that must be one of `asc` or `desc`. The default `order` is `asc`. | ||
|
|
||
| If elements in `target` are |
There was a problem hiding this comment.
What are your thoughts on permanently converting the types of values in heterogeneous arrays vs. just converting the types for comparison? I think it would be fairly straightforward to only convert the types within the sort function and leave the actual type in the array. I can see arguments for both sides here, but want to make sure we're considering our options.
There was a problem hiding this comment.
I see changing the types permanently and sorting a list as two distinct intentions. Both are reasonable actions in data processing. Here, we provide better flexibility to users. If they want to change the types permanently, they can simply overwrite the attributes by using set(). Otherwise, they will still have the original array on their hands
There was a problem hiding this comment.
That makes sense, thanks. If we see changing the types and sorting as separate actions, then shouldn't we keep the original types of the array elements when sorting? Right now we do:
[true, 1, 2.3, "str"] --> ["1", "2.3", "str", "true"]
When I think it would be possible to do:
[true, 1, 2.3, "str"] --> [1, 2.3, "str", true]
There was a problem hiding this comment.
I see your point. Yes, preserving the types may be more intuitive. I thought keeping the sort result as a string to make it clear that the comparison is done as a string. It sounds good to maintain the original type.
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
- remove redundant order check - return error for unsupported types
- change visibility of type constraint
…tor-contrib into ottl_sort_func # Conflicts: # pkg/ottl/ottlfuncs/README.md
There was a problem hiding this comment.
Thanks for your continued efforts @kaisecheng. This mostly looks good to me.
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
|
@evan-bradley Many thanks for reviews and suggestions. I believe this is ready to merge now. |
|
@TylerHelmuth Please take a look. |
pkg/ottl/e2e/e2e_test.go
Outdated
| @@ -619,6 +619,24 @@ func Test_e2e_converters(t *testing.T) { | |||
| tCtx.GetLogRecord().Attributes().PutStr("test", "d74ff0ee8da3b9806b18c877dbf29bbde50b5bd8e4dad7a3a725000feb82e8f1") | |||
| }, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
Can you add a test for a mixed-type slice?
There was a problem hiding this comment.
This is a pretty complex function, I'd like to see more e2e tests that cover the different type scenarios it supports
There was a problem hiding this comment.
added more e2e test covering unit types and mixed types
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
@evan-bradley @TylerHelmuth Do you have any additional thoughts or feedback that I can address? |
|
This still looks good to me. @TylerHelmuth please take a look when you are able. |
**Description:** <Describe what has changed.> Add `Sort` function to sort array to ascending order or descending order `Sort(target, Optional[order])` The Sort Converter preserves the data type of the original elements while sorting. The behavior varies based on the types of elements in the target slice: | Element Types | Sorting Behavior | Return Value | |---------------|-------------------------------------|--------------| | Integers | Sorts as integers | Sorted array of integers | | Doubles | Sorts as doubles | Sorted array of doubles | | Integers and doubles | Converts all to doubles, then sorts | Sorted array of integers and doubles | | Strings | Sorts as strings | Sorted array of strings | | Booleans | Converts all to strings, then sorts | Sorted array of booleans | | Mix of integers, doubles, booleans, and strings | Converts all to strings, then sorts | Sorted array of mixed types | | Any other types | N/A | Returns an error | Examples: - `Sort(attributes["device.tags"])` - `Sort(attributes["device.tags"], "desc")` **Link to tracking Issue:** <Issue number if applicable> open-telemetry#34200 **Testing:** <Describe what testing was performed and which tests were added.> - Unit tests - E2E tests **Documentation:** <Describe the documentation added.> readme for Sort function --------- Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
Description:
Add
Sortfunction to sort array to ascending order or descending orderSort(target, Optional[order])The Sort Converter preserves the data type of the original elements while sorting.
The behavior varies based on the types of elements in the target slice:
Examples:
Sort(attributes["device.tags"])Sort(attributes["device.tags"], "desc")Link to tracking Issue:
#34200
Testing:
Documentation:
readme for Sort function