-
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
feat(tsi): optimize series iteration #20544
feat(tsi): optimize series iteration #20544
Conversation
seriesOpt := opt | ||
if len(opt.Dimensions) == 0 { | ||
// no point ordering the series if we are just aggregating them | ||
seriesOpt.Ordered = false |
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.
Not sure on this particular check - maybe needs to be more granular depending on the type of call iterator?
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.
Yeah, I think there might be some call iterators that need sorting.
tsdb/engine/tsm1/engine.go
Outdated
@@ -2588,6 +2649,18 @@ func (e *Engine) createVarRefIterator(ctx context.Context, measurement string, o | |||
return nil, nil | |||
} | |||
|
|||
// check for optimized series iteration for tsi index | |||
if e.index.Type() == tsdb.TSI1IndexName { |
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.
This has a unit test but I'm not sure there's actually a way to hit it from an influxql query without opt.Ordered=true
, so might need to remove this section.
Note we also have the |
b4bc184
to
2174d54
Compare
Now with gofmt run |
We're going to do this in flux instead of influxql, so this can be closed. |
@benbjohnson We're back to planning on doing this in influxql, so this PR is back on the menu. |
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.
Overall I think the code makes sense. I agree that disabling sorting when there is a group by may not always be safe. Otherwise 👍
seriesOpt := opt | ||
if len(opt.Dimensions) == 0 { | ||
// no point ordering the series if we are just aggregating them | ||
seriesOpt.Ordered = false |
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.
Yeah, I think there might be some call iterators that need sorting.
2174d54
to
ab9c1f6
Compare
When using queries like 'select count(_seriesKey) from bigmeasurement`, we should iterate over the tsi structures to serve the query instead of loading all the series into memory up front. Closes influxdata#20543
ab9c1f6
to
98a76a1
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.
May not require changes, but just a verification of the questions I ask below: should a cursor that failed to open be closed, and should iterators that failed to open not be closed.
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.
Thanks for assuaging my doubts.
When using queries like 'select count(_seriesKey) from bigmeasurement`, we
should iterate over the tsi structures to serve the query instead of loading
all the series into memory up front.
Closes #20543
Describe your proposed changes here.