-
Notifications
You must be signed in to change notification settings - Fork 85
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
Merge the query process of series index #505
Conversation
ButterBright
commented
Aug 4, 2024
- If this pull request closes/resolves/fixes an existing issue, replace the issue number. Fixes [BanyanDB-Server] Optimizing Series Index Querying Performance skywalking#12239.
Pleaae update e2e tool version to fix CI |
pkg/index/inverted/inverted.go
Outdated
for i := range fieldKeys { | ||
fields = append(fields, fieldKeys[i].Marshal()) | ||
} | ||
// TODO: add trace information |
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.
Should we finish todo in this pr?
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 agree. We should trace the query before merging it.
} | ||
// TODO: add node for Query |
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.
Same question here.
pkg/index/inverted/query.go
Outdated
@@ -49,30 +49,22 @@ type GlobalIndexError struct { | |||
|
|||
func (g GlobalIndexError) Error() string { return g.IndexRule.String() } | |||
|
|||
var _ index.Query = (*Query)(nil) | |||
|
|||
// Query is a wrapper for bluge.Query. | |||
type Query struct { |
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.
Do you need to expose this type since its operations are wrapped by the index.Query
?
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.
Nice job
What is the new query performance benchmark after this? |
Sorry for the late reply. I'm adding the relevant benchmarks. Theoretically, the topology queries should see a significant improvement. |