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.

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

[processor/sumologicschema]: add nesting processor #877

Merged
merged 5 commits into from
Dec 22, 2022

Conversation

aboguszewski-sumo
Copy link
Contributor

@aboguszewski-sumo aboguszewski-sumo commented Dec 20, 2022

Updates #865
We're waiting for a response on the issue, but besides that, this is finished.

@github-actions github-actions bot added documentation Improvements or additions to documentation go labels Dec 20, 2022
@aboguszewski-sumo aboguszewski-sumo force-pushed the json-deflattener branch 2 times, most recently from ffa6f9c to 9048ad6 Compare December 22, 2022 10:25
Comment on lines 152 to 180
#### Known issues

This feature has undefined behavior when in input there are various keys that will be mapped to the same nested structure.
For example, given the following attributes:

```json
{
"a.b.c.": "d",
"a": {
"b": {
"c": "e"
}
}
}
```

It is not possible to predict, what will be the result. It will have the following structure:

```json
{
"a": {
"b": {
"c": ...
}
}
}
```

However, it is not possible to know a priori if the value under key `c` will be equal to `d` or `e`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is one issue that I encountered and I think that we should definitely make it a UB.

Copy link
Contributor

Choose a reason for hiding this comment

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

what is happening in current implementation?
IMHO, We shouldn't override existing keys

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, keys are overwritten. If there was a map, the maps are merged (for example "a.b": "c" and "a": {"d":"e"} get merged into "a": {"b": "c", "d":"e"}).
The problem is that we can't easily decide whether we will be overwriting a key, because it depends on the order in which Range method of pcommon.Map traverses the attributes. We would have to make some kind of preprocessing to check all existing keys.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense, I think for the first iteration it should be fine. Let's follow the KISS principle

@aboguszewski-sumo aboguszewski-sumo marked this pull request as ready for review December 22, 2022 12:07
@aboguszewski-sumo aboguszewski-sumo requested a review from a team as a code owner December 22, 2022 12:07
Comment on lines +35 to +45

# Specifies if attributes should be nested, basing on their keys.
# See "Nesting attributes" documentation chapter from this document.
nest_attributes:
# Defines whether attributes should be nested.
# default = false
enabled: {true, false}

# Defines the string used to separate key names in attributes that are to be nested.
# default = "."
separator: <separator>
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, we could have some pattern matching here. So in case customer wants to leave some keys which has separator in it he can do i, but current behavior seems to be reasonable for first iteration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been already commented this on Slack. IMO we should add this in another PR (it is also arguable what should the filters be: we can do an allowlist or a denylist, with advanced matching options or not... imo it's the best to just add a denylist of prefixes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation go
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants