[query] Fix time() function in binary comparisons #1888
Conversation
arnikola
changed the title
src/query/parser/promql/types.go
Outdated
| } | ||
|
|
||
| func isTime(expr promql.Expr) bool { | ||
| return expr.String() == "time()" |
There was a problem hiding this comment.
Hm, surely there's a better way to work out of it's a time expr? Isn't there a type safe enum like value you can use?
There was a problem hiding this comment.
Not before it gets parsed unfortunately; alternatively could do some weird stuff where it attempts to edit the parent's params if you receive a time() function, but I think that breaks the model in a worse way than this. Figured since parsing is going to be messy and special-casey anyway it won't be a huge difference
There was a problem hiding this comment.
Ok actually going to refactor it to make it a little less awkward; will rely on block types rather than on prom's param types and remove that janky dependency
src/query/parser/promql/types.go
Outdated
| // NB: Prometheus handles `time()` in a way that's not entirely compatible | ||
| // with the query engine; special handling is required here. | ||
| lTime, rTime := isTime(expr.LHS), isTime(expr.RHS) | ||
| if matching == nil { |
There was a problem hiding this comment.
nit: Could we make promMatchingToM3 return *thing, bool instead of using nil to specify it didn't convert the expr? Just tends to be the pattern we use rather than checking whether thing is nil or not (which can lead to some people forgetting to nil check, and then nil ptr dereference).
src/query/parser/promql/types.go
Outdated
|
|
||
| // Iff one of left or right is a time() function, match match one to many | ||
| // against it, and match everything. | ||
| func timeMatcher(left, right bool) *binary.VectorMatching { |
There was a problem hiding this comment.
Similar, maybe here we make it return (*binary.VectorMatching, bool)?
Even if we don't always use the return of the bool side, still makes the top level code look better:
matching, ok := promMatchingToM3(expr.VectorMatching)
if !ok {
matching, _ = timeMatcher(isTime(expr.LHS), isTime(expr.RHS))
}| "github.com/m3db/m3/src/query/models" | ||
| "github.com/stretchr/testify/assert" | ||
|
|
||
| "github.com/uber-go/tally" |
There was a problem hiding this comment.
nit: Combine the external imports here? (don't know why linter is clearly not working...)
i.e. this should be:
"context"
"testing"
"github.com/m3db/m3/src/query/cost"
"github.com/m3db/m3/src/query/models"
"github.com/stretchr/testify/assert"
"github.com/uber-go/tally"
There was a problem hiding this comment.
Haha can't say I miss the linter no longer breaking builds, but will fix it up
src/query/block/information.go
Outdated
| func (t BlockType) String() string { | ||
| switch t { | ||
| case BlockM3TSZCompressed: | ||
| return "M3TSZ_compressed" |
There was a problem hiding this comment.
Any reason this should be upper case and rest lower? Maybe we should make it just "compressedM3TSZ" for lower camel casing or "compressed_m3tsz" if we want to do lowercase underscore separated.
There was a problem hiding this comment.
SGTM; don't really see this used as anything other than as human-readable debug info since it's all runtime stuff so makes sense to pretty it up 👍
src/query/block/information.go
Outdated
| @@ -0,0 +1,86 @@ | |||
| // Copyright (c) 2019 Uber Technologies, Inc. | |||
There was a problem hiding this comment.
super nit: Rename this file info.go for more "Go-ish" name?
src/query/block/information.go
Outdated
| return "unknown" | ||
| } | ||
|
|
||
| type BlockInformation struct { |
There was a problem hiding this comment.
Should we call this just BlockInfo? Sounds more "Go-ish" (I know that's so subjective obviously though...).
Also needs comment yeah? (think the exported types in this file are all missing comments)
There was a problem hiding this comment.
Ah fair enough; thought the general idea was functions and types have full names but variables have Go-ish names, will change it
| BlockTime | ||
| BlockContainer | ||
| BlockWrapper | ||
| BlockConsolidated |
There was a problem hiding this comment.
These all needs comments yeah?
src/query/functions/binary/common.go
Outdated
| @@ -107,6 +76,34 @@ func tagMap(t models.Tags) map[string]models.Tag { | |||
| return m | |||
| } | |||
|
|
|||
| // Iff one of left or right is a time block, match match one to many | |||
| // against it, and match everything. | |||
| func defaultVectorMatcherBuilder(lhs, rhs block.Block) *VectorMatching { | |||
There was a problem hiding this comment.
Can we just make VectorMatching be a non-ptr type? Then it shouldn't panic if accidentally used without being set.
There was a problem hiding this comment.
Sure, was mostly using it for the unset case, will add a bool indicating that it's been updated
|
|
||
| "github.com/m3db/m3/src/query/block" | ||
|
|
||
| "github.com/m3db/m3/src/query/executor/transform" |
There was a problem hiding this comment.
nit: missing endline here?
| @@ -22,7 +22,8 @@ package promql | |||
|
|
|||
| import ( | |||
| "fmt" | |||
| "time" | |||
|
|
|||
| "github.com/m3db/m3/src/query/block" | |||
|
|
|||
| "github.com/m3db/m3/src/query/functions" | |||
There was a problem hiding this comment.
nit: extra new line here?
src/query/block/types.go
Outdated
| @@ -167,6 +182,7 @@ func (m Metadata) String() string { | |||
| type Builder interface { | |||
| AppendValue(idx int, value float64) error | |||
| AppendValues(idx int, values []float64) error | |||
| SetBlockType(blockType BlockType) | |||
There was a problem hiding this comment.
Instead of using SetBlockType can we just force it to be supplied in the constructor to the builder?
Seems like people could forget otherwise rather than forcing them to supply in the constructor.
There was a problem hiding this comment.
Discussed offline re: adding a BuildAsType method to Builder instead to make this more explicit as most usages of Build want default typing
What this PR does / why we need it:
Fixes #1878