Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[pkg/ottl] add support for map literals #34168

Merged
merged 13 commits into from
Aug 12, 2024
Merged

[pkg/ottl] add support for map literals #34168

merged 13 commits into from
Aug 12, 2024

Conversation

bacherfl
Copy link
Contributor

@bacherfl bacherfl commented Jul 19, 2024 •
edited
Loading

Description: This PR extends the OTTL grammar to support map literals

Link to tracking Issue: #32388

Testing: Unit tests; E2E Tests

Documentation: Extended the docs with a section about the added data type

@bacherfl bacherfl marked this pull request as ready for review July 22, 2024 07:38
@bacherfl bacherfl requested a review from a team July 22, 2024 07:38
@bacherfl
Copy link
Contributor Author

@evan-bradley this one should be ready for review now

evan-bradley
evan-bradley reviewed Jul 22, 2024
View reviewed changes
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really excited to see this one, thanks for taking this on.

pkg/ottl/expression.go Outdated Show resolved Hide resolved
pkg/ottl/grammar.go Outdated Show resolved Hide resolved
pkg/ottl/grammar.go Outdated Show resolved Hide resolved
evan-bradley
evan-bradley approved these changes Jul 22, 2024
View reviewed changes
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me outside a request regarding the map type.

pkg/ottl/expression.go Outdated Show resolved Hide resolved
TylerHelmuth
TylerHelmuth reviewed Jul 22, 2024
View reviewed changes
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another question I'd like discussed in the issue: should map literals be indexable in the grammar? I would think not, since we don't allow that for slice literals either.

pkg/ottl/grammar.go Show resolved Hide resolved
@evan-bradley evan-bradley mentioned this pull request Jul 22, 2024
Closed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 8, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Aug 8, 2024
@evan-bradley
Copy link
Contributor

@TylerHelmuth I think this one is still ready whenever you have time.

@evan-bradley evan-bradley removed the Stale label Aug 8, 2024
TylerHelmuth
TylerHelmuth reviewed Aug 9, 2024
View reviewed changes
pkg/ottl/lexer_test.go Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've added new tokens so we need to add tests that recognize them.

pkg/ottl/parser_test.go
@@ -89,6 +89,115 @@ func Test_parse(t *testing.T) {
WhereClause: nil,
},
},
{
name: "editor with map",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test for an empty map

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also an editor with a converter with a map.

TylerHelmuth
TylerHelmuth reviewed Aug 9, 2024
View reviewed changes
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally new getters means new tests in functions_test.go. Do we need one here as well?

@bacherfl
additional tests
67b8ce3 
Signed-off-by: Florian Bacher <[email protected]>
@bacherfl
Copy link
Contributor Author

Normally new getters means new tests in functions_test.go. Do we need one here as well?

Looking at those tests it seems they are only testing the functionality of the exported getters (e.g. PMapGetter, StringLikeGetter etc.) with the underlying getter function being replaced with a mocked implementation. As the mapGetter added in this PR is not an exported one, I think adding a test case in functions_test.go might not be required (there also is no test case for the listGetter which would be an equivalent of the mapGetter)

@bacherfl
Copy link
Contributor Author

Thank you for the review @TylerHelmuth - I added additional test cases where applicable

@TylerHelmuth TylerHelmuth merged commit 951d6f8 into open-telemetry:main Aug 12, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 12, 2024
@bacherfl bacherfl mentioned this pull request Aug 19, 2024
Closed
6 tasks
f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this pull request Sep 12, 2024
@bacherfl @f7o
[pkg/ottl] add support for map literals (open-telemetry#34168)
041bce9 
**Description:** This PR extends the OTTL grammar to support map
literals

**Link to tracking Issue:** open-telemetry#32388

**Testing:** Unit tests; E2E Tests

**Documentation:** Extended the docs with a section about the added data
type

---------

Signed-off-by: Florian Bacher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan-bradley evan-bradley evan-bradley approved these changes

@TylerHelmuth TylerHelmuth TylerHelmuth approved these changes

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu bogdandrutu is a code owner

@kentquirk kentquirk Awaiting requested review from kentquirk

Assignees

@andrzej-stencel andrzej-stencel

Labels
pkg/ottl
Projects
None yet
Milestone
v0.107.0
Development

Successfully merging this pull request may close these issues.
4 participants
@bacherfl @evan-bradley @TylerHelmuth @andrzej-stencel