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 Count distinct support #741 #1073

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lgualtieri75
Copy link
Contributor

tests in PR #1066

Copy link
Collaborator

@jussisaurio jussisaurio left a comment

Choose a reason for hiding this comment

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

The bytecode plan for this differs quite significantly from SQLite -- SQLite uses an ephemeral index and the Found instruction to skip rows where the key is already found in the ephemeral index. This PR's implementation relies on creating an owned string for every value and then conditionally inserting the stringified value to a HashSet.

Moreover, I don't think specialcasing COUNT(DISTINCT foo) is a terribly good idea - what about AVG(DISTINCT foo), SUM(DISTINCT foo) and so on? What about DISTINCT without an aggregation? There should be a single distinctness mechanism and the aggregate function should not be tightly coupled to it.

Unfortunately having such a mechanism depends on us having an OpenEphemeral implementation which we do not yet have.

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.

2 participants