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

Record API should support grouping results by a given field. #216

Closed
kgodey opened this issue Jun 8, 2021 · 10 comments · Fixed by #315
Closed

Record API should support grouping results by a given field. #216

kgodey opened this issue Jun 8, 2021 · 10 comments · Fixed by #315
Assignees
Labels
ready Ready for implementation type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL

Comments

@kgodey
Copy link
Contributor

kgodey commented Jun 8, 2021

Problem

The user should have the option to show records grouped by a given field. We need API support for the UI to show this.

Proposed solution

The API should support this use case, in addition to filtering and sorting options. See #157 and #158. Specifically, we need an API that provides group counts of various values (see comments below).

Additional context

As part of this issue, we'll need to define what a grouped API response looks like. Please update the API standards with details and request review from @kgodey and @mathemancer before proceeding.

@kgodey kgodey added type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL ready Ready for implementation labels Jun 8, 2021
@mathemancer
Copy link
Contributor

Do we need a "grouped view" from the API? I was envisioning:

  1. ask API for group keys based on column set.
  2. ask API for records matching given group key (i.e., filtered so that the involved columns match the requisite values)

If the front end wants multiple groups, couldn't it just ask for those groups serially? I guess we could implement some kind of 'give me multiple matching sets for multiple filters' endpoint, but it seems like that logic belongs in the front-end, since it'll know (for example) how many rows it wants, or how many rows per group it wants to show, etc.

@kgodey
Copy link
Contributor Author

kgodey commented Jun 9, 2021

I'd like @pavish's input on this. My concerns is that one API call per group might slow the frontend down since there's way more network requests to make (especially for groups where there's only one or two records per group... that would be 25-50 network requests for a page size of 50 records).

@kgodey kgodey added status: detail needed and removed ready Ready for implementation labels Jun 9, 2021
@kgodey
Copy link
Contributor Author

kgodey commented Jun 9, 2021

Changing status until questions are resolved.

@pavish
Copy link
Member

pavish commented Jun 12, 2021

I do not think we should do one API call per group, as this would lead to a lot of requests as @kgodey mentioned.

The response eventually depends on the UX. The 'grouped view' we are discussing for the UI is basically a sorted view. It does not perform the actual 'group by' query on the db.

If we do not have to show the count, we can show the 'grouped view' by using the sort functionality. In order to show the count, we will have to do point 1 of what @mathemancer mentioned (ask API for group keys based on column set, with count), but this is costly and needs to be thought out.

@kgodey
Copy link
Contributor Author

kgodey commented Jun 14, 2021

@pavish we do show the count, according to the UX in the Filter, Sort, and Group tables. If we have a special API endpoint for grouped views, we can provide the count of each group from the backend.

@pavish
Copy link
Member

pavish commented Jun 14, 2021

@kgodey That sounds good. We can have a separate API endpoint that provides only the count of each group, probably better if we have a query param to filter the groups.

This will be the flow:

  1. Table has a column called status, and user is grouping by that column.
  2. Frontend sends a request sorting by status column, and shows the result in a grouped view after fetching.
  3. Frontend requests the separate API for the count of the visible groups.
  4. Upon pagination, 2 & 3 are repeated.

The reason we are not fetching all group counts at the start is because, as of now we do not have a web socket functionality to know if a table is updated. Since data may get updated, having a cached set of counts on the frontend would lead to inconsistency.

@kgodey
Copy link
Contributor Author

kgodey commented Jun 14, 2021

Sounds good @pavish, I updated the main issue description.

@eito-fis
Copy link
Contributor

@kgodey Some questions before I get started on this:

  • Do we want to have the visible groups passed to the endpoint, or should they be calculated? The former is more flexible and probably more efficient, but the latter could take the same exact request as the record request. Given the ease of use on the front end, I assume the latter would be nicer.
  • Where should this endpoint go? I'm imagining v0/tables/<id>/groups/.
  • Related, are we worried at all about the database updating between the two requests? I could also see passing group_counts=true to the records list endpoint and bundling the group info into the response. (Although, I guess variable responses aren't a good thing?)

@kgodey
Copy link
Contributor Author

kgodey commented Jun 30, 2021

@eito-fis

  • I think they should be calculated, I like the idea of taking the same request as the record request (plus what columns to group by)
  • I think bundling the group info into the record response could work, if we always included keys for grouping in the response. For instance, we could return something like this in the metadata section of the response (with count, etc.) if the response was grouped:
{
  "group_count_by": "teacher_name", 
  "group_count": [
    "John Smith": 3,
    "Jane Doe": 12
  ]
}

If it wasn't grouped, we could do something like:

{
  "group_count_by": null, 
  "group_count": null
}

That would avoid problems with both database changes and variable responses.

@eito-fis
Copy link
Contributor

Awesome, I'll plan on taking in an additional group_count_by parameter at the record list endpoint and returning the values as described.

@kgodey kgodey added ready Ready for implementation and removed status: detail needed labels Jun 30, 2021
@eito-fis eito-fis mentioned this issue Jul 1, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Ready for implementation type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants