-
Notifications
You must be signed in to change notification settings - Fork 22
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
Fix/large groups #263
Fix/large groups #263
Conversation
This reverts commit 8784699. Temporarily reverting this commit to get tests to pass
Add ungrouped aggregations; group before ordering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small indentation thing and a couple questions about testing in CLJS. But looks good overall!
qry '{:context {:ex "http://example.org/ns/"} | ||
:select [?name (count ?favNums)] | ||
:where [[?s :schema/name ?name] | ||
[?s :ex/favNums ?favNums]]}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this line needs to be unindented one space
@@ -0,0 +1,32 @@ | |||
(ns fluree.db.query.aggregate-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to test this in CLJS anymore?
@@ -0,0 +1,29 @@ | |||
(ns fluree.db.query.fql-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to test this in CLJS too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... a couple questions about testing in CLJS ...
Funny you should mention that. These are actually new test files in these branches. I reflexively added them as cljc files, and they fail on the cljs side.
The other newly added json-ld tests are all clj only, so I decided not to let the cljs failures hold back this merge because there's a lot of code here, so I decided to make those new files clj only, and I'm planning on opening another issue to make the cljs tests work for all the new json-ld stuff too.
group-by queries were broken for queries with more than 2 grouped fields (the query would just hang with no error message). This patch makes those queries behave like the smaller grouping queries and adds a test.
I also added a minor speed improvement by using
peek
instead oflast
becausepeek
optimally gets the last item from a vector, wherelast
does it in linear time by converting the vector to a sequence first. I threw in a benchmarking lib in the development profile that will come in handy as we add/refactor more code.In fixing this bug I have uncovered some behavior that looks weird to me regarding grouped results. If there is one field that has a single value over the grouping grouped with another field that has multiple values, the single value is repeated. For example, if we group by person, then email will have 1 value across the grouping but favorite numbers has multiple. So the output will repeat email as many times as there are favorite numbers. You can see this behavior in the test that I added. I'm not positive if this is wrong, or what the correct behavior is, but I will consult the sparql spec and see how other dbms's handle this case and possibly submit another pr depending on what I find.