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 ungrouped aggregations; group before ordering #265

Merged
merged 24 commits into from
Nov 28, 2022

Conversation

zonotope
Copy link
Contributor

@zonotope zonotope commented Nov 28, 2022

This patch computes ungrouped queries with aggregation functions as if all results are in a single implicit group, supporting queries like:

'{:context  {:ex "http://example.org/ns/"}
  :select   [(count ?name)]
  :where    [[?s :schema/name ?name]]}

This patch also changes the grouping algorithm to avoid needing to sort the result set if there's no order-by clause specified.

This builds on #263, so take a look at that one first.

closes #239

@zonotope zonotope requested a review from a team November 28, 2022 15:24
Copy link
Contributor

@bplatz bplatz left a comment

Choose a reason for hiding this comment

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

I think this looks really good. Not sure if you have a sense on why the test is failing.

Realize this is a draft, but would be great to get it committed since it touches highly volatile files at the moment, don't want to create a more challenging time getting it merged later (or vice versa for other merges).

@zonotope zonotope marked this pull request as ready for review November 28, 2022 15:46
@zonotope
Copy link
Contributor Author

Not sure if you have a sense on why the test is failing.

This is a new (or, at least newly uncommented) test. It's failing in cljs, but I reflexively made it a cljc file when all the other tests are clj.

I've changed it to a clj file and will add an issue to make the tests we need work in cljs.

@zonotope zonotope merged commit f7f5f5d into fix/large-groups Nov 28, 2022
@zonotope zonotope deleted the feature/group-by branch November 28, 2022 18:28
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