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

Query planner should validate distinct is passed a field #2612

Closed
pauldix opened this issue May 19, 2015 · 8 comments · Fixed by #2622
Closed

Query planner should validate distinct is passed a field #2612

pauldix opened this issue May 19, 2015 · 8 comments · Fixed by #2622
Assignees
Milestone

Comments

@pauldix
Copy link
Member

pauldix commented May 19, 2015

When the user does a select distinct query, it should validate that the argument passed to distinct is a field. If it's a tag it should return an error like: select distinct must be passed a field. To query the unique values for a tag use "show tag values from measurement_name with key = 'key_name'"

Where measurement_name and key_name are replaced with whatever was in the query.

@corylanou
Copy link
Contributor

Currently, they fail as expected:

> select distinct host from mem
ERR: host isn't a field on measurement mem
> select distinct(host) from mem
ERR: host isn't a field on measurement mem
> select count(distinct host) from mem
ERR: host isn't a field on measurement mem

However, I suspect once we merge #2598 that it will stop working. If that is the case, I have to wait for that PR to get finished before I can tackle this one?

@pauldix
Copy link
Member Author

pauldix commented May 19, 2015

I think you can get to this one before that PR gets merged. The validation would still happen in the query planning stage when looking at what is passed into distinct.

The only thing I would say is that the error message should be more helpful since there is a way to get at what they're asking for i.e. show tag values from foo with key = 'bar'

@corylanou
Copy link
Contributor

Ok, I was thinking more about testing. I can write a unit test which should be fine, but I won't be able to finish out integration tests which will leave the issue open until the other PR is merged.

@pauldix
Copy link
Member Author

pauldix commented May 19, 2015

Ah right you are

@toddboom
Copy link
Contributor

Still seems to be working after #2598:

> select distinct host from "mydb"."default"."cpu_load_short";
ERR: host isn't a field on measurement cpu_load_short
> select distinct(host) from "mydb"."default"."cpu_load_short";
ERR: host isn't a field on measurement cpu_load_short
> select count(distinct host) from "mydb"."default"."cpu_load_short";
ERR: host isn't a field on measurement cpu_load_short

@corylanou
Copy link
Contributor

There is a distinction between a SELECT DISTINCT foo and SELECT count(DISTINCT foo) As they are different intentions, you will get a custom error for the first, and the general for the second. The thinking is that you can't do aggregate functions on tags, but you can do select distincts, just not with that syntax (as the error will inform you).

So now, if you issue either of these queries (where host is a tag, not a field):

SELECT distinct(host) FROM cpu
SELECT distinct host FROM cpu

You will get an error like:

host isn't a field on measurement cpu; to query the unique values for a tag use SHOW TAG VALUES FROM cpu WITH KEY = "host"

If you issue a query like this:

SELECT count(distinct host) FROM cpu
SELECT count(distinct(host)) FROM cpu

You will get the generic message like:

host isn't a field on measurement cpu

@toddboom
Copy link
Contributor

that seems fine to me

@pauldix
Copy link
Member Author

pauldix commented May 20, 2015

@corylanou that looks right. Eventually we'd want them to be able to do a count(distinct ...) on a tag value. Maybe update the error to be host isn't a field on measurement cpu. count(distinct) on tags isn't yet supported

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 a pull request may close this issue.

3 participants