-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Undesirable record grouping behaviours #352
Comments
Do we also want the |
The |
@pavish A couple of questions: (1) Are you suggesting that we move this functionality to a separate API or that we fix the current API? It's unclear from the issue description. For context, the idea behind keeping it in the record list API was that we could return group counts for a given query. We would only look at the list of records that the passed-in limit, offset, sorts, and filters affect and return group counts for that query. For example, if you were displaying page 2 of a table with 50 records per page, we would show you the group counts relevant to groups visible on that page. @eito-fis I assume this matches your understanding. (2) If we do move it to a separate API, I'm not sure what you mean by "It could also obtain the values for columns, to filter the grouped results." Could you explain further, including how you would use it? |
(1) I am open to both options. I suggested moving to separate API, as the group_counts currently are not related to the record results.
This would solve all the problems at hand. I also assumed this to be the behaviour initially, but this isn't the current behaviour. For this, we would need to fetch the records first, then find the values for which we obtain the grouped counts, with only the filters and not anything else. But I believe currently, it is directly applying all the query params on the (2) If we had a dedicated API, we would need something like |
Let's go with keeping it in the current API and fixing it to work the way I described. @eito-fis let me know if you have any questions. |
To be clear, its not that we don't want to consider limit or offset at all, rather we want to limit and offset before grouping. Is this correct? |
@eito-fis Yes. |
@kgodey @eito-fis There is one other change, it's more on the convention side of the API. Currently the results are in this format, when there are more than one grouped column:
I think it would make more sense to change it to:
I am siding with the latter, since column names could have commas in them. |
@pavish I find that format confusing, since both I suggest we do something like: {
"results": [
{
"columns": ["NASA Kennedy Space Center", "Application"],
"count": 29
},
{
"columns": ["NASA Kennedy Space Center", "Issued"],
"count": 58
}
]
} |
@kgodey In this scenario, if page 2 has only one group 'Group A' which in total consists of 90 records, I assumed this would return the group name 'Group A' with count 90. But currently limit is still considered, and it returns 50. We would need to re-open this issue. |
I think this was my error with speccing it out, I didn't think through what considering limits would mean. Since @eito-fis is out this week, I'll prioritize fixing it. |
Actually, since this isn't blocking @pavish (the counts will just be wrong for now), I'm not going to prioritize it myself. |
Description
Record grouping has a set of behaviours, that are not desirable.
It considers order_by, which leads to formation of incorrect query on the backend, if we don't group by the sorted column.
It considers limit and offset. These apply on the grouped result itself, and is unrelated to the record limit & offset.
Expected behavior
We could also probably have a dedicated API for this. It could also obtain the values for columns, to filter the grouped results. Having it as part of records API makes less sense, since the group count is not a reflection of the record results.
The text was updated successfully, but these errors were encountered: