Query engine rewrite #1600
Query engine rewrite #1600
Conversation
pauldix
force-pushed
the
fix-raw-ordering
branch
from
b0965d0
to
89b80dc
Compare
toddboom
changed the title
…tch new query engine output.
| @@ -600,6 +600,7 @@ func runTestsData(t *testing.T, testName string, nodes Cluster, database, retent | |||
| if name == "" { | |||
| name = tt.query | |||
| } | |||
| fmt.Printf("TEST: %d: %s\n", i, name) | |||
There was a problem hiding this comment.
Is this deliberate? Seems like it just adds noise to the tests.
There was a problem hiding this comment.
oh, I had it so that if there's a failure and you're tracing through, you can find that test along with whatever output was coming out of the logs then. Was very helpful for me troubleshooting something.
There was a problem hiding this comment.
Valid requirement, but you can get this with go test -v I believe -- it's what I do.
There was a problem hiding this comment.
You don't, the problem is that test -v Doesn't put the test info inline with the actual output of the server. Basically, you get all the server output, and then at the end you get the test output. So it's impossible to see which maps with what. With this you see the info about the test running with the server output. Besides, this only shows up if you run with -v
| for _, id := range m.seriesIDs { | ||
| seriesIdsToExpr[id] = nil | ||
| } | ||
| return seriesIdsToExpr | ||
| } | ||
| ids, _, _ := m.walkWhereForSeriesIds(stmt.Condition, seriesIdsToExpr) | ||
|
|
||
| // ids will be empty if all they had was a time in the where clause. so return all measurement series ids | ||
| if len(ids) == 0 && stmt.OnlyTimeDimensions() { |
| @@ -332,7 +369,7 @@ func (m *Measurement) idsForExpr(n *influxql.BinaryExpr) (seriesIDs, bool, influ | |||
| } | |||
|
|
|||
| // ignore time literals | |||
| if _, ok := value.(*influxql.TimeLiteral); ok { | |||
| if _, ok := value.(*influxql.TimeLiteral); ok || name.Val == "time" { | |||
There was a problem hiding this comment.
How could this happen? Either it's a TimeLiteral or not, right? Also doing a string-compare? Is that necessary?
There was a problem hiding this comment.
If they do a WHERE time > 1s then it ends up going to this. This is just during the planning phase of the query so a string compare isn't really that big of a deal.
There was a problem hiding this comment.
Should WHERE time = 'military' be a valid expression?
There was a problem hiding this comment.
no, that should return a parse error because that's not a valid time.
|
One thing I need to add to this are tests in |
| for _, k := range keys { | ||
| r.fn(k, data[k], e) | ||
| default: | ||
| // we shouldn't get here, but give them back nils if it goes this way |
There was a problem hiding this comment.
Quietly accepting stuff that shouldn't happen always makes me nervous. A panic might seem harsh, but if it shouldn't happen, shouldn't we find out?
There was a problem hiding this comment.
I'd rather give back nils than panic.
avoid unnecessary call to walkWhereForSeriesIds
influxql: change 2 methods to funcs
|
Going to live dangerously and merge this in. Please continue to review and I'll update based on feedback in a new PR. Need to get the fixes this adds out in an RC tonight for some testing. |
This PR is for a refactor/rewrite of the query engine. There's no additional functionality, but this refactor will enable future work to fix the following:
I also aimed to simplify how the query engine worked. Instead of using channels to pass values from the map and reduce functions, they now just return values. This also makes it possible to do smaller sets of results at a time since the query engine can now manipulate how much data each call to the mapper will churn through.
I also pulled all of the stuff for defining functions into its own file. This should be the one place people look to when wiring up new query functions.