Improve (date)_range just like filters
#970
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
trzysiek
force-pushed
the
semifix-for-all-combinators
branch
from
730be6c to
3f8dabd
Compare
trzysiek
changed the title
(date)_range just like filters(date)_range just like filters
trzysiek
force-pushed
the
semifix-for-all-combinators
branch
from
5e674e7 to
f1064ec
Compare
trzysiek
changed the title
(date)_range just like filters(date)_range just like filters
Deploying quesma with
|
| Latest commit: |
3b9d891
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a258b9d5.quesma.pages.dev |
| Branch Preview URL: | https://semifix-for-all-combinators.quesma.pages.dev |
trzysiek
requested review from
nablaone,
jakozaur,
mieciu,
avelanarius and
pdelewski
trzysiek
commented
Nov 13, 2024
Comment on lines
+56
to
+59
| if test.TestName == "Line, Y-axis: Min, Buckets: Date Range, X-Axis: Terms, Split Chart: Date Histogram(file:kibana-visualize/agg_req,nr:9)" { | ||
| t.Skip("Date range is broken, fix in progress (PR #971)") |
There was a problem hiding this comment.
Adding a new test that fails. Unskipping another one above/below as this PR fixes it.
trzysiek
commented
Nov 13, 2024
Comment on lines
+433
to
443
| if !isCombinator { | ||
| return | ||
| } | ||
|
|
||
| noMoreBucket := len(pancake.layers) <= 1 || (len(pancake.layers) == 2 && pancake.layers[1].nextBucketAggregation == nil) | ||
| noMetricOnFirstLayer := len(firstLayer.currentMetricAggregations) == 0 && len(firstLayer.currentPipelineAggregations) == 0 | ||
| canSimplyAddCombinatorToWhereClause := noMoreBucket && noMetricOnFirstLayer | ||
| if canSimplyAddCombinatorToWhereClause { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
I think code should be a bit clearer now? Feel free to disagree
There was a problem hiding this comment.
Though not sure if that's always correct if we count size. Though it might be.
There was a problem hiding this comment.
Ok, we add size as metric aggregation, so this should be good.
trzysiek
force-pushed
the
semifix-for-all-combinators
branch
from
f068003 to
db2bd9c
Compare
jakozaur
approved these changes
Nov 21, 2024
Comment on lines
+433
to
443
| if !isCombinator { | ||
| return | ||
| } | ||
|
|
||
| noMoreBucket := len(pancake.layers) <= 1 || (len(pancake.layers) == 2 && pancake.layers[1].nextBucketAggregation == nil) | ||
| noMetricOnFirstLayer := len(firstLayer.currentMetricAggregations) == 0 && len(firstLayer.currentPipelineAggregations) == 0 | ||
| canSimplyAddCombinatorToWhereClause := noMoreBucket && noMetricOnFirstLayer | ||
| if canSimplyAddCombinatorToWhereClause { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Though not sure if that's always correct if we count size. Though it might be.
Comment on lines
+433
to
443
| if !isCombinator { | ||
| return | ||
| } | ||
|
|
||
| noMoreBucket := len(pancake.layers) <= 1 || (len(pancake.layers) == 2 && pancake.layers[1].nextBucketAggregation == nil) | ||
| noMetricOnFirstLayer := len(firstLayer.currentMetricAggregations) == 0 && len(firstLayer.currentPipelineAggregations) == 0 | ||
| canSimplyAddCombinatorToWhereClause := noMoreBucket && noMetricOnFirstLayer | ||
| if canSimplyAddCombinatorToWhereClause { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Ok, we add size as metric aggregation, so this should be good.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Don't get dissuaded by the nr of changed lines, there's almost nothing happening here.
(Date)_rangeandfiltersare basically the same aggregation. I recently improvedfiltersa bit, but didn't do anything to(date)_range.This PR makes all those combinators work in the same way, so there are two issues left - #971 (easier) and #944 (a bit harder). Afterwards they will all fully work.