Fix interpolation for data points at bucket boundaries#139798
Fix interpolation for data points at bucket boundaries#139798kkrik-es merged 12 commits intoelastic:mainfrom
Conversation
|
Hi @kkrik-es, I've created a changelog YAML for you. |
| 11.13218 | 3339.65266 | 2024-05-10T00:10:00.000Z | prod | ||
|
|
||
| ; | ||
|
|
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
martijnvg
left a comment
There was a problem hiding this comment.
LGTM 👍 . I only left questions.
| if (lastTsSec == firstTsSec) { | ||
| // Check for the case where there is only one sample in state, right at the boundary towards a non-empty adjacent state. | ||
| if (state.samples == 1) { | ||
| if (previousState != null) { |
There was a problem hiding this comment.
Is is possible that both previousState and nextState is null or that they both are not null?
There was a problem hiding this comment.
In that case is this allowed and then maybe do else if block?
There was a problem hiding this comment.
If they are both null, we just return Nan below - can't do better for now.
If they are both not null, we'd get a value to the other boundary so lastTsSec == firstTsSec would be false. I'll add asserts for this.
| * Rate and increase calculations use interpolation at the boundaries between time buckets | ||
| */ | ||
| RATE_WITH_INTERPOLATION, | ||
| RATE_WITH_INTERPOLATION_V2, |
There was a problem hiding this comment.
This is just to make bwc tests pass, right? Cause once this is merged and back ported, the no mode will report RATE_WITH_INTERPOLATION?
There was a problem hiding this comment.
Correct for bwc. I didn't use the new tag on unaffected tests, but can do so if you prefer.
| pr: 139798 | ||
| summary: Fix interpolation for data points at bucket boundaries | ||
| area: ES|QL | ||
| type: bug |
There was a problem hiding this comment.
I think we can make this issue an non-issue, given that this bug isn't released?
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
💔 Backport failed
You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
* Fix flakiness in TSDataGenerationHelper * Fix interpolation for data points at bucket boundaries * indentation * Update docs/changelog/139798.yaml * [CI] Auto commit changes from spotless * fix csv tests * asserts and date factor * update doc --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> (cherry picked from commit b6ead64) # Conflicts: # muted-tests.yml
#139850) * Fix interpolation for data points at bucket boundaries (#139798) * Fix flakiness in TSDataGenerationHelper * Fix interpolation for data points at bucket boundaries * indentation * Update docs/changelog/139798.yaml * [CI] Auto commit changes from spotless * fix csv tests * asserts and date factor * update doc --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> (cherry picked from commit b6ead64) # Conflicts: # muted-tests.yml * Add muted test for RandomizedRollingUpgradeIT
Interpolation logic didn't cover the case where there's a single data point at the lower boundary and just a previous state, or a single point at the upper boundary and just a next state. If this happens, the data point matches the only interpolated value, so there are no 2 data points within the state to calculate the delta. The fix is to use the delta from the closest data point in the adjacent state.
While there, added the following fixes to the randomized testing suite:
Fixes #139732