Skip to content
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

Add EXACT CARDINALITY commands #8984

Merged
merged 12 commits into from
Oct 26, 2017
Merged

Add EXACT CARDINALITY commands #8984

merged 12 commits into from
Oct 26, 2017

Conversation

e-dard
Copy link
Contributor

@e-dard e-dard commented Oct 19, 2017

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated

This PR introduces five new commands:

SHOW SERIES EXACT CARDINALITY [ON] [FROM] [WHERE] [GROUP BY] [LIMIT] [OFFSET]
SHOW MEASUREMENT EXACT CARDINALITY [ON] [FROM] [WHERE] [GROUP BY] [LIMIT] [OFFSET]
SHOW FIELD KEY EXACT CARDINALITY [ON] [FROM] [WHERE] [GROUP BY] [LIMIT] [OFFSET]
SHOW TAG KEY EXACT CARDINALITY [ON] [FROM] [WHERE] [GROUP BY] [LIMIT] [OFFSET]
SHOW TAG VALUES EXACT CARDINALITY [ON] [FROM] WITH KEY = "x" [WHERE] [GROUP BY] [LIMIT] [OFFSET]

Further, two versions of both the already-implemented SHOW SERIES CARDINALITY and SHOW MEASUREMENT CARDINALITY commands have been converted to utilise the HLL++ sketches that are maintained in the index. These estimations are very accurate, and very fast.

Specifically, the variants that currently support cardinality estimation are

SHOW SERIES CARDINALITY;
SHOW SERIES CARDINALITY ON db0;
SHOW MEASUREMENT CARDINALITY
SHOW MEASUREMENT CARDINALITY ON db0;

All other SHOW X CARDINALITY commands will continue to use the exact-counting approaches of the SHOW X EXACT CARDINALITY commands. They will be converted to estimations in future releases.

}

return []*models.Row{&models.Row{
Columns: []string{"cardinality"},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benbjohnson @jsternberg question over what your thoughts are on this name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think cardinality works for me. 👍

@e-dard e-dard added this to the 1.4.0 milestone Oct 20, 2017
}

return []*models.Row{&models.Row{
Columns: []string{"cardinality"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think cardinality works for me. 👍

@ghost ghost assigned e-dard Oct 23, 2017
@ghost ghost added the review label Oct 23, 2017
@e-dard e-dard force-pushed the er-show-cardinality branch 2 times, most recently from ce571d2 to f1947fb Compare October 24, 2017 11:04
influxql/ast.go Outdated
_, _ = buf.WriteString(" ON ")
_, _ = buf.WriteString(QuoteIdent(s.Database))
}
return buf.String()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me like you should do this instead.

if s.Exact {
   _, _ = buf.WriteString(" EXACT")
}
_, _ = buf.WriteString(" CARDINALITY")

if s.Database != "" {
    // the stuff that's here
}

if !s.Exact {
    return buf.String()
}

Then there's less code repetition.

influxql/ast.go Outdated
_, _ = buf.WriteString("SHOW MEASUREMENT CARDINALITY")
_, _ = buf.WriteString("SHOW MEASUREMENT")

if !s.Exact {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same comment from above applies here.

@e-dard e-dard force-pushed the er-show-cardinality branch from 49d204e to f7e0e41 Compare October 26, 2017 15:22
@e-dard
Copy link
Contributor Author

e-dard commented Oct 26, 2017

@jsternberg changes made. Thanks.

@e-dard e-dard merged commit 26cc46d into master Oct 26, 2017
@e-dard e-dard deleted the er-show-cardinality branch October 26, 2017 16:16
@ghost ghost removed the review label Oct 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants