MQE: introduce ResultType method for query planner Node
#11132
Merged
Conversation
This file contains hidden or 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
jhesketh
reviewed
Apr 7, 2025
| @@ -128,3 +129,15 @@ func (f *FunctionCall) OperatorFactory(children []types.Operator, timeRange type | |||
| return nil, compat.NewNotSupportedError(fmt.Sprintf("'%v' function", f.FunctionName)) | |||
| } | |||
| } | |||
|
|
|||
| func (f *FunctionCall) ResultType() (parser.ValueType, error) { | |||
| if _, ok := functions.InstantVectorFunctionOperatorFactories[f.FunctionName]; ok || f.FunctionName == "absent" || f.FunctionName == "absent_over_time" { | |||
There was a problem hiding this comment.
Bit of a nit, but I dislike we have to keep this special case in multiple places.
We should add a comment at least.
But I wonder if we can make this better. Maybe some kind of dummy entry in functions.InstantVectorFunctionOperatorFactories ?
There was a problem hiding this comment.
I worry that a dummy entry in functions.InstantVectorFunctionOperatorFactories might cause other problems or confusion.
I'm not expecting to need further instances of this special case anywhere else, if that helps allay your concerns.
There was a problem hiding this comment.
I think if it panics when used incorrectly it could work, but I'm nit picking so I'm okay with it as is, but would love a comment on the line please.
jhesketh
approved these changes
Apr 7, 2025
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.
What this PR does
This PR introduces a
ResultTypemethod for the query plannerNodeinterface.This will allow optimization passes to correctly handle nodes that produce different kinds of values.
I'm raising this as a separate PR in the interests of keeping the upcoming common subexpression elimination PR smaller.
Which issue(s) this PR fixes or relates to
(none)
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.