Skip to content

implement support for count(_, :distinct) #171

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

Merged
merged 2 commits into from
Jun 20, 2025

Conversation

aseigo
Copy link
Contributor

@aseigo aseigo commented Jun 20, 2025

See #169

This appears to work. Tried it out on a database I have, as well as ensuring the tests pass. But perhaps @rhcarvalho could test this branch as well on their use case?

@rhcarvalho
Copy link

Hey thanks! The implementation looks straightforward, I wonder if there are any gotchas as @warmwaffles hinted in the issue🤔

@warmwaffles
Copy link
Member

@rhcarvalho I think this is okay. What I think I need is a test with an obscure distinct in it.

@warmwaffles
Copy link
Member

warmwaffles commented Jun 20, 2025

@aseigo try removing this line in your PR

# Distinct with options not supported
:distinct_count,

This will pick up the integration tests for distinct counts.

@aseigo
Copy link
Contributor Author

aseigo commented Jun 20, 2025

Integration tests enabled, and they work.

And yes, I'm wondering if there is some weirdo edge case lurking as well .. at least we can maybe find out this way! :)

@aseigo
Copy link
Contributor Author

aseigo commented Jun 20, 2025

Hey thanks! The implementation looks straightforward, I wonder if there are any gotchas as @warmwaffles hinted in the issue🤔

Have you managed to give it a try with your query in your codebase? It would be nice to have "real world" confirmations!

@warmwaffles
Copy link
Member

If it passes those integration tests, I'm happy. I don't remember the edge case, but I'm sure if someone finds it, they'll open a ticket and we can fix it 😛

@warmwaffles warmwaffles merged commit a214e91 into elixir-sqlite:main Jun 20, 2025
11 checks passed
@warmwaffles
Copy link
Member

I'm gonna make a cut this evening for both exqlite and the adapters.

@rhcarvalho
Copy link

Thank you @aseigo and @warmwaffles! Shipping to production today.

@warmwaffles
Copy link
Member

@rhcarvalho just keep an eye out for errors please and report back findings.

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