Repository navigation
Fix to tdigest implicit eval - #140690
Fix to tdigest implicit eval#140690
Conversation
felixbarny
left a comment
There was a problem hiding this comment.
Please also test convert functions on non-histogram fields and also nested convert functions.
|
|
||
| // ----------------------------------------- | ||
|
|
||
| Core supported t-digest aggs on histogram field, without grouping (TS mode) |
There was a problem hiding this comment.
We should add more TS mode tests here, but this is kind of the minimum to fix this specific bug.
| * @return true iff the given predicate is true for all nodes | ||
| */ | ||
| public boolean allMatch(Predicate<? super T> predicate) { | ||
| return anyMatch(Predicate.not(predicate)) == false; |
There was a problem hiding this comment.
@alex-spies I discussed this briefly with you, but tagging you here in case you want to see the implementation.
|
Okay, BWC tests are failing trying to load the histogram time series data, because the histogram metric type did not exist prior to 9.3. This probably needs some way to gate loading that data set for older versions; |
JonasKunz
left a comment
There was a problem hiding this comment.
LGTM after applying Felix' suggestion.
…t-implicit-eval' into fix-to-tdigest-implicit-eval
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
Hi @not-napoleon, I've created a changelog YAML for you. |
Fixes elastic#140670 This addresses the linked bug, but it's not at all clear to me that the solution is correct. I'm pushing this up as a draft to discuss. If we agree that this is the right way to fix it, I can write some more tests for it and get the PR merged. Also, it turns out we didn't have any TS mode CSV tests for histogram data. I've added a new CSV data source and one test for that case. We need to replicate all the other tests in TS mode too, I think. --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>
Fixes #140670
This addresses the linked bug, but it's not at all clear to me that the solution is correct. I'm pushing this up as a draft to discuss. If we agree that this is the right way to fix it, I can write some more tests for it and get the PR merged.
Also, it turns out we didn't have any TS mode CSV tests for histogram data. I've added a new CSV data source and one test for that case. We need to replicate all the other tests in TS mode too, I think.