-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Improve SHOW TAG KEYS performance. #9073
Conversation
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.
Just a couple of suggestions.
coordinator/statement_executor.go
Outdated
@@ -932,6 +934,98 @@ func (e *StatementExecutor) executeShowSubscriptionsStatement(stmt *influxql.Sho | |||
return rows, nil | |||
} | |||
|
|||
func (e *StatementExecutor) executeShowTagKeys(q *influxql.ShowTagKeysStatement, ctx *query.ExecutionContext) error { | |||
var dis []meta.DatabaseInfo | |||
if q.Database != "" { |
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 don't think that q.Database
can be empty. The StatementRewriter
should have set it I think. Did you encounter a case where it was empty?
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 you're right. Not sure where I was thinking it could be empty. Fixed in 156f25a.
coordinator/statement_executor.go
Outdated
} | ||
dis = []meta.DatabaseInfo{*di} | ||
} else { | ||
dis = e.MetaClient.Databases() |
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 don't think SHOW TAG VALUES
should work across databases.
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.
Fixed in 156f25a.
tsdb/store.go
Outdated
switch e := e.(type) { | ||
case *influxql.BinaryExpr: | ||
// Remove time clause from condition | ||
if ref, ok := e.LHS.(*influxql.VarRef); ok && strings.ToLower(ref.Val) == "time" { |
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.
The time
clause should already be removed by influxql.ConditionExpr
in the StatementExecutor
I think.
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.
Removed in 156f25a.
f1408bc
to
156f25a
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.
LGTM 👍
Required for all non-trivial PRs