Refactor QueryExecutor #5663
Refactor QueryExecutor #5663
Conversation
| ) | ||
|
|
||
| // NewQueryExecutor returns an instance of QueryExecutor with registered statement executors. | ||
| func NewQueryExecutor(mc *meta.Client, s *tsdb.Store, m *monitor.Monitor, pw *cluster.PointsWriter) *influxql.QueryExecutor { |
There was a problem hiding this comment.
PointsWriter not needed? Was confused why this was getting passed into a query executor
benbjohnson
force-pushed
the
query-executor
branch
from
aae8dac to
64e7bb4
Compare
| StatementNormalizer StatementNormalizer | ||
|
|
||
| // Rewrites statements into other statements, as needed. | ||
| StatementRewriter *StatementRewriter |
There was a problem hiding this comment.
Why is this exposed and a pointer, or even a named type? StatementRewriter is defined as an empty struct, and the (pointer) receiver value is never used, other than to call methods. This field should be taken out and non-exported functions should be used instead.
benbjohnson
force-pushed
the
query-executor
branch
5 times, most recently
from
cb8b570 to
aad9e9b
Compare
|
This PR is ready for review. There is one outstanding issue where the |
|
Looks fine to me so far, but the sections that are commented out should either be fixed if they are needed or removed if they are no longer needed. |
benbjohnson
force-pushed
the
query-executor
branch
from
aad9e9b to
9bebc2b
Compare
|
@jsternberg I implemented a smaller wrapper type for |
?
| } else if got, exp := len(res.Series), 1; got != exp { | ||
| t.Fatalf("unexpected response.\n\ngot: %d series\nexp: %d\n", got, exp) | ||
| } | ||
| // FIXME(benbjohnson): Convert to metadata reads. |
There was a problem hiding this comment.
Weird. I swear I removed these sections. This needs to be tested at the QueryExecutor level so I wasn't going to rewrite them here.
This commit moves the `QueryExecutor` to the `cluster` package and provides an interface to it inside the `influxql` package.
benbjohnson
force-pushed
the
query-executor
branch
from
9bebc2b to
e3b4b71
Compare
|
👍 |
|
LGTM 👍 |
Refactor QueryExecutor (WIP)
Overview
This pull request moves QueryExecutor from the
tsdbpackage to theinfluxqlpackage and adds additionalStatementExecutorinterfaces for each statement type.TODO
SET PASSWORDSELECT INTOmetatest package/cc @jwilder @dgnorton @joelegasse @e-dard