feat: use count_hll for 'show series cardinality' queries #20745
Conversation
| @@ -2001,7 +2008,7 @@ func (is IndexSet) MeasurementSeriesKeyByExprIterator(name []byte, expr influxql | |||
| } | |||
| if ids == nil { | |||
| release() | |||
| return nil, nil | |||
| release = nil | |||
There was a problem hiding this comment.
For us to successfully use a nil measurementSeriesKeyByExprIterator, we would have to return a boxed nil, which is ugly.
Instead just set the ids to nil and the methods can check that.
Closes: influxdata#20614 Also fix nil pointer for seriesKey iterator Fix for bug in: influxdata#20543 Also add a test for ingress metrics
lesam
force-pushed
the
fix-nil-pointer-series-iterator
branch
from
62301f3 to
6711ca6
Compare
| @@ -202,6 +202,30 @@ func rewriteShowSeriesCardinalityStatement(stmt *influxql.ShowSeriesCardinalityS | |||
| } | |||
| } | |||
|
|
|||
| // do hll estimation for per-measurement queries | |||
| if !stmt.Exact && stmt.Dimensions == nil && stmt.Limit == 0 && stmt.Offset == 0 { | |||
There was a problem hiding this comment.
I could take these conditions off (except exact) but I think it would muddy the waters.
count_hll works for any kind of query, but the optimized series iteration only works for no dimension/limit/offset.
| @@ -1982,7 +1982,8 @@ func (itr *measurementSeriesKeyByExprIterator) Next() ([]byte, error) { | |||
| } | |||
|
|
|||
| func (itr *measurementSeriesKeyByExprIterator) Close() error { | |||
| if itr == nil { | |||
There was a problem hiding this comment.
Do you want to get rid of the itr == nil check? Would that be impossible? Having both seems more defensive.
There was a problem hiding this comment.
I think I'd prefer the panic - nobody should be using a nil, and we usually don't check it I think.
There was a problem hiding this comment.
Dave Cheney is on my side here, which is good enough for me:
Tip: Don’t check for a nil receiver, employ high test coverage and vet/lint tools to spot unhandled error conditions resulting in nil receivers.
There was a problem hiding this comment.
Unfortunately, we have that panic happening, and I have to figure it out. It looks like a failed cleanup from the creation of multiple iterators, which are all then closed, and some are nil while others were created.....
| @@ -1933,7 +1933,7 @@ type measurementSeriesKeyByExprIterator struct { | |||
| } | |||
|
|
|||
| func (itr *measurementSeriesKeyByExprIterator) Next() ([]byte, error) { | |||
| if itr == nil { | |||
| if itr.ids == nil { | |||
There was a problem hiding this comment.
As below on Close() - perhaps check for itr == nil as well.
Closes #20614
In addition to the query rewrites, fix a recent bug in seriesKey iteration exposed by the cardinality tests,
and add an extra tests for the recent ingress metric work.