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

Remove upfront check whether an instant query is splittable or not #2574

Merged
merged 3 commits into from
Jul 28, 2022

Conversation

pracucci
Copy link
Collaborator

What this PR does

Currently we need to change logic in 2 places when we add support to split more type of queries: both isSplittable() function and the actual instantSplitter logic.

I want to remove isSplittable() and let go every single query through the mapping logic. At the end of the mapping, we already check if it was successfully split or not, to decide whether it will be executed by the query-frontend (if was split) or just fully run it downstream (if wasn't split).

Which issue(s) this PR fixes or relates to

N/A

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Copy link
Contributor

@ssncferreira ssncferreira left a comment

Choose a reason for hiding this comment

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

LGTM 🎬
Just a comment about the impact of this change

switch e := expr.(type) {
case *parser.AggregateExpr:
// A vector aggregation is splittable, if the aggregation operation is supported and the inner expression is also splittable.
return supportedVectorAggregators[e.Op] && isSplittable(e.Expr)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this will introduce problems 🤔 (need to think a bit more about this).
But by removing this we are opening the door to all expressions wrapped by vector aggregators to be split.
Maybe it won't be a problem because we only send downstream the splittableVectorAggregators vectors 🕵️‍♀️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, let's investigate if this could cause issues. If it does, then we should port this logic to instantSplitter.mapAggregatorExpr().

In instantSplitter.mapAggregatorExpr() we have:

if i.outerAggregationExpr == nil && splittableVectorAggregators[expr.Op] {
mappedExpr, finished, err = NewASTExprMapper(i.copyWithEmbeddedExpr(expr)).MapExpr(expr.Expr, stats)
} else {
mappedExpr, finished, err = i.MapExpr(expr.Expr, stats)
}

Since splittableVectorAggregators is already stricter, the risk of this change could from here:

mappedExpr, finished, err = i.MapExpr(expr.Expr, stats)

Copy link
Contributor

@colega colega left a comment

Choose a reason for hiding this comment

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

As long as the tests pass, this should be fine 🤷

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Copy link
Contributor

@ssncferreira ssncferreira left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci enabled auto-merge (squash) July 28, 2022 15:11
@pracucci pracucci merged commit df621ac into main Jul 28, 2022
@pracucci pracucci deleted the do-not-upfront-if-instant-query-is-splittable branch July 28, 2022 15:25
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.

None yet

3 participants