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

Adding document counts to the query API #16578

Open
hyzx86 opened this issue Aug 18, 2024 · 12 comments
Open

Adding document counts to the query API #16578

hyzx86 opened this issue Aug 18, 2024 · 12 comments
Milestone

Comments

@hyzx86
Copy link
Contributor

hyzx86 commented Aug 18, 2024

Is your feature request related to a problem? Please describe.

Currently, some OC query APIs such as LuceneQuery , SqlQuery and graphQL Query do not yet support statistical aggregation.

Describe the solution you'd like

  1. Lucene Query Elastic Search Query
    These api's make it easy to implement statistics. We just need to refactor the graphQL provider associated with them. 2. SqlQuey In SqlQuery we should allow users to write SQL for statistical queries.

  2. SqlQuey
    In SqlQuery, we should allow the user to write SQL for statistical queries, perhaps with additional script fields, or some other better way of accomplishing this, and the current API doesn't seem to support it.

  3. the script function executeQuery, whose return value is currently always a collection.

Describe alternatives you've considered

If we can't implement these features anytime soon, I suggest that before the 2.0 release, we first return IQueryResult objects in all locations where we need to call queryManager.ExecuteQueryAsync to return data, instead of just returning the collection.

@hishamco
Copy link
Member

Can you propose a PR?

@hyzx86
Copy link
Contributor Author

hyzx86 commented Aug 18, 2024

Can you propose a PR?

I'd like to discuss the feasibility of these features first, since I put up a related PR many years ago, which seemed to be unaccepted

#6939
#11159

#5693

@hyzx86 hyzx86 closed this as completed Aug 18, 2024
@hyzx86 hyzx86 reopened this Aug 18, 2024
@MikeAlhayek
Copy link
Member

I think having a count is useful specially if you need to create a pager. Is there a reason why this was rejected? Does all the search providers gives you a way to get a count without returning the list of contentItemId?

@hyzx86
Copy link
Contributor Author

hyzx86 commented Aug 21, 2024

Does all the search providers gives you a way to get a count without returning the list of contentItemId?

From my understanding of OC,
Lucene Query already includes totals, we just have not open them up,

SqlQuery definitely doesn't support them, if we need to support them, my suggestion is to provide a switch for the query to query for count, When the user turns on this option, the page will display another SQL editor for the user to fill in the logic of how to count

The grpahql field should support statistics, but obviously needs to be reorganised, or use Connections as described at #5693.

@sebastienros
Copy link
Member

  1. I believe it shouldn't be an issue to expose and extra field for the count, but this should be opt-in, existing queries should not get the count by default or this could impact the perf. It has to be on-demand.
  2. Can't you already create SQL queries that return any kind of data?
  3. We could have a new executeScalarQuery to return an object

@sebastienros
Copy link
Member

Should these be 3 distinct issues?

@sebastienros sebastienros added this to the 2.x milestone Aug 29, 2024
Copy link
Contributor

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

@hyzx86
Copy link
Contributor Author

hyzx86 commented Aug 30, 2024

Should these be 3 distinct issues?

  1. I believe it shouldn't be an issue to expose and extra field for the count, but this should be opt-in, existing queries should not get the count by default or this could impact the perf. It has to be on-demand.我相信公开计数的额外字段应该不是问题,但这应该是选择加入的,现有查询默认不应获取计数,否则这可能会影响性能。它必须是按需的。

Yes I couldn't agree more, in graphQL field provider we can check if the user needs the total field to do a statistic query.

  1. Can't you already create SQL queries that return any kind of data?难道您已经创建了返回任何类型数据的 SQL 查询吗?

Not at the moment, but I made a Jint-based Script Query that can return objects, or data in any format.

  1. We could have a new executeScalarQuery to return an object我们可以有一个新的 executeScalarQuery 来返回一个对象

Yes, I added an executeObjectQuery to support ScriptQuery calls.

@hyzx86
Copy link
Contributor Author

hyzx86 commented Aug 30, 2024

Should these be 3 distinct issues?这应该是 3 个不同的问题吗?

The main point of bringing up this issue is to suggest that we check where we are calling queries that return collections instead of objects for subsequent extensions.

Since it's the point in time when 2.0 is ready for release, and we've already done a lot of disruptive upgrades in OC 2.0,
I'd suggest adding these tweaks to the 2.0 release schedule now

@PBMikeW
Copy link
Contributor

PBMikeW commented Oct 16, 2024

I had to write a custom controller to get the total results back (needed for paging!) Elastic and Lucene already include a total count in LuceneQueryResults and ElasticQueryResults. So the total on those is effectively free, we could include an optional total on SQL results.

Most document databases are not that big, however mine has over 400k and returning a count on all published documents takes 900ms, and all of my biggest contenttype is 400ms. So expensive but I would never use SQL for those.

public class LuceneQueryResults : IQueryResults
{
    public IEnumerable<object> Items { get; set; }

    public int Count { get; set; }
}

@Skrypt
Copy link
Contributor

Skrypt commented Oct 16, 2024

image

Here is how I fixed your code example 😉

@yqzhen1990
Copy link
Contributor

image

Here is how I fixed your code example 😉

image

If I want to get Count in the liquid template for use in paging, how can I do it?
I noticed that only result.Items is returned here:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants