-
Notifications
You must be signed in to change notification settings - Fork 54
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
execution: start implementing subqueries #280
execution: start implementing subqueries #280
Conversation
155de0c
to
7205782
Compare
7205782
to
1bbd6c2
Compare
7068708
to
50a8f50
Compare
benchmark results from the one contrived query:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as a first step, let's see if anyone else has any comments or suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, just one question from my side.
As a side note, I found the refactoring to query.Options a bit distracting from the main change introduced by the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@MichaHoffmann is this good to merge? |
ee69a0e
to
c7e52a4
Compare
Yeah! I rebased on latest main and it passes promql acceptance tests too ! |
c7e52a4
to
44eabd4
Compare
Signed-off-by: Michael Hoffmann <[email protected]>
44eabd4
to
5ad544b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's also worth adding how subqueries support works like to README 😄
Yeah, valid remark; Ill add a small section in a new PR in a minute |
caveats: