fix(thor): Fix error filtering before range aggregations - #20560
Conversation
trevorwhitney
left a comment
trevorwhitney
left a comment
There was a problem hiding this comment.
looks good, my only question is if we would rather concatenate in the case of multiple errors? is there a good reason not to do this and always prefer the one on the right?
There was a problem hiding this comment.
my understanding of the problem that was fixed here was the input already has an __error__ and __error_details__ column, correct? if so, can we add a test for that case?
| fromVal := from.Value(i) | ||
| if fromVal != "" { | ||
| builder.Append(fromVal) | ||
| } else { | ||
| builder.Append(to.Value(i)) | ||
| } | ||
| } |
There was a problem hiding this comment.
why overwrite the existing error instead of concatenating? would it be better to return all the errors we could, or are you worried that would get messy concatenated them together?
There was a problem hiding this comment.
changed to a merge
trevorwhitney
left a comment
trevorwhitney
left a comment
There was a problem hiding this comment.
looks good, thanks for the change to mergeErrors!
3 participants
What this PR does / why we need it:
I realized that I implemented
avgand groupingsbyandwithout, but forgot to enable them. When I enabled them some tests started failing on a weird thing. After a long investigation it turned out that:jsonparse andunwrapstages can add__error__columns into the record, which leads to column duplicate and ambiguity for other stages in processing/filtering rows with errors.error == ""which returnedfalseon nulls.This PR has following changes:
avgand various combinations of vector and range aggregations.__error__columns inprojectstage.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR