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

fix(reduce transform): improve array handling #3076

Merged
merged 3 commits into from Jul 16, 2020
Merged

Conversation

lukesteensen
Copy link
Member

Closes #3074

Two bugs fixed here:

  1. Using all_fields instead of into_iter gaves us paths instead of keys, so thing like arrays and objects would not get strategized correctly.
  2. Special casing arrays when constructing the ArrayMerger meant that the initial array would be flattened in the result but subsequent additions would not.

Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
@@ -77,12 +77,11 @@ impl ReduceState {
fn new(e: LogEvent, strategies: &IndexMap<String, MergeStrategy>) -> Self {
Self {
stale_since: Instant::now(),
// TODO: all_fields alternative that consumes
Copy link
Contributor

Choose a reason for hiding this comment

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

As it was foretold.

r#"
identifier_fields = [ "request_id" ]

merge_strategies.foo = "array"
Copy link
Contributor

Choose a reason for hiding this comment

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

How about one for concat?

Copy link
Contributor

@Hoverbear Hoverbear left a comment

Choose a reason for hiding this comment

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

I think this needs to consider the concat strategy as well!

@lukesteensen
Copy link
Member Author

I think this needs to consider the concat strategy as well!

I actually started down this path and backed out because it seemed slightly out of scope. The main thing that stopped me was that the current concat collects into a BytesMut and adding a second array "flavor" of concat adds a lot of edge cases where behavior will depend on the type of the first value seen, etc. We can totally do it, it just started to look more complex than I initially assumed.

Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
@Hoverbear
Copy link
Contributor

Ok that's fine with me!

Copy link
Contributor

@Hoverbear Hoverbear left a comment

Choose a reason for hiding this comment

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

Seems to fix the issue!

Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
@lukesteensen
Copy link
Member Author

Just kidding, it wasn't that bad. Just pushed an implementation.

@Hoverbear
Copy link
Contributor

@lukesteensen lukesteensen merged commit 0b9e4cc into master Jul 16, 2020
@lukesteensen lukesteensen deleted the fix-array-reduce branch July 16, 2020 00:02
mengesb pushed a commit to jacobbraaten/vector that referenced this pull request Dec 9, 2020
Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
Signed-off-by: Brian Menges <brian.menges@anaplan.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Reduce transform has unexpected behavior on arrays
2 participants