-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat: Add count aggregate support #102
Conversation
810e7de
to
e8ecf93
Compare
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 few comments/thoughts/issues.
Biggest gripe is the implementation of aggregate funcs as individual planNodes. We can talk about this during the next call.
"github.com/sourcenetwork/defradb/query/graphql/parser" | ||
) | ||
|
||
type countNode struct { |
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.
I mentioned this briefly in the convo on #28... My reservation about making each aggregate func its own dedicated node is that it will inflate the plan graph notably for queries that use a notable number of aggregates. Especially if youre thinking about composing aggregates out of other aggregates (IE: Avg from Sum and Count).
Especially if to implement a single aggregate func, we need to define an entire planNode with methods that have very little to do with the process of aggregation.
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.
I don't worry so much about the size of the graph, and the graph would remain as flat as it would otherwise be (something I do see as fairly important). They are independent, and there should be very little overhead involved, even when composing - avg could use the output from sum and count inline (with the downside of having to ensure sum and count are run first in the planner).
But we should talk later on this as you said.
b1dfea3
to
804f4d0
Compare
I believed we agreed to defer the architectural decision on many vs one aggregate node(s) until we have at least one more node. I've added a comment on this in the countNode, planner and selectNode |
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.
Comments/responses all look good to me! Stoked to have the first aggregate in!
804f4d0
to
63d9397
Compare
* Add count aggregate support to object querys * Add count aggregate support to commit queries
Closes #93
Adds count aggregate support within object and commit queries. Adjusts the render system slightly, introducing a new
_hidden
field allowing records to be yielded through nodes, but not outputted to the query consumer (in this case, it saves a lot of hassle when counting with a limit/offset on the child collection).