ISPN-14194 Add config property for default max result #10378

fax4ever · 2022-10-10T12:33:03Z

https://issues.redhat.com/browse/ISPN-14194

~~Just the first step, that is adding the config property for it.~~ ready for the review
I'm writing the documentation

domiborges · 2022-10-11T16:02:57Z

core/src/main/resources/schema/infinispan-config-14.0.xsd

+        <xs:attribute name="default-max-result" type="xs:integer" default="${Query.default-max-result}">
+          <xs:annotation>
+            <xs:documentation>
+              Applies the given value as maxResults as a default to any query (indexed, non-indexed and hybrid).


A suggestion:
Limits the number of results returned by a query. Applies to indexed, non-indexed, and hybrid queries. Setting the default-max-results significantly improves performance of queries that don't have an explicit limit set.

Looks good to me

tristantarrant · 2022-10-12T09:25:41Z

The name of the property should be plural (default-max-results)

fax4ever · 2022-10-13T12:24:28Z

Thank you for the review @dvagnero and @tristantarrant, going to apply the changes.
In the meantime I have completed the implementation, but we still need to address the documentation.

fax4ever · 2022-10-14T10:06:32Z

I applied the changes and fixed the last stuff. I'm writing my doc additions.

tristantarrant · 2022-10-14T10:27:27Z

query/src/main/java/org/infinispan/query/impl/QueryDefinition.java

@@ -104,6 +111,10 @@ public SearchQueryBuilder getSearchQueryBuilder() {
      return searchQueryBuilder;
   }

+   public boolean maxResultsHasChanged() {


I don't like the name. Maybe isCustomMaxResults() ?

good idea. changed it

tristantarrant · 2022-10-14T10:33:07Z

core/src/main/java/org/infinispan/configuration/cache/QueryConfiguration.java

+ */
+public class QueryConfiguration extends AbstractTypedPropertiesConfiguration implements Matchable<QueryConfiguration> {
+
+   public static final AttributeDefinition<Integer> DEFAULT_MAX_RESULTS = AttributeDefinition.builder(org.infinispan.configuration.parsing.Attribute.DEFAULT_MAX_RESULTS, 100).immutable().build();


For now, immutable is fine, but in the future we could turn this into a runtime-mutable attribute. Can you create a Jira for it ?

tristantarrant

One small change in a method name. Otherwise LGTM

tristantarrant · 2022-10-14T10:34:43Z

We may want to add some query benchmarks to https://github.com/infinispan/infinispan-benchmarks

fax4ever · 2022-10-14T13:56:29Z

thanks @tristantarrant for the review!
I made the changes and created the follow up: https://issues.redhat.com/browse/ISPN-14232

fax4ever · 2022-10-14T15:26:15Z

@dvagnero I made my attempt to document the (restored) behavior and the new property here: ISPN-14194 Document query default max results

fax4ever · 2022-10-14T15:27:15Z

@wfink I don't know if you want to take a look at it

domiborges · 2022-10-14T16:04:43Z

documentation/src/main/asciidoc/topics/upgrading.adoc

+== Query efficiency
+
+Restore the default query max results that used to be present in older Infinispan versions (prior of 12.1.5.Final)
+and provide a new configuration attribute to change this default, that is now restored to 100.
+Limiting the number of results returned by a query. Applies to indexed, non-indexed, and hybrid queries.
+It significantly improves performance of queries that don't have an explicit limit set.


Suggested change

== Query efficiency

Restore the default query max results that used to be present in older Infinispan versions (prior of 12.1.5.Final)

and provide a new configuration attribute to change this default, that is now restored to 100.

Limiting the number of results returned by a query. Applies to indexed, non-indexed, and hybrid queries.

It significantly improves performance of queries that don't have an explicit limit set.

== Query efficiency

The `default-max-results` property that was present in Infinispan 12.1.5.Final and earlier versions has been restored.

This property limits the number of results returned by a query and significantly improves performance of queries that don't have an explicit limit set.

The default value of `default-max-results` is 100.

Not quite @dvagnero : Infinispan never had a default-max-results property. It just had a default, unmodifiable value for it.

Thanks @dvagnero, I merged them:

The default query max results that was present in Infinispan 12.1.5.Final and earlier versions has been restored,
together with the fact that you can now also change this default by setting the query.default-max-results cache property.
Limiting the number of results returned by a query significantly improves performance of queries that don't have an explicit limit set.
The default value of default-max-results is 100.

@dvagnero what do you think of?

Thanks! What about...
The default for maximum results returned by a query that was present in Infinispan 12.1.5.Final and earlier versions has been restored. You can now change the default limit by setting the default-max-results cache property. The default value of default-max-results is 100.
Limiting the number of results returned by a query significantly improves performance of queries that don't have an explicit limit set.

sounds great to me... I'm applying it. Thanks

domiborges · 2022-10-14T16:28:49Z

documentation/src/main/asciidoc/topics/ref_query_ickle.adoc

+The default limit of `100` is applied if `maxResults` is not explicitly set to the query instance.
+You can change this default for all the queries by setting the cache property `query.default-max-result`.


Suggested change

The default limit of `100` is applied if `maxResults` is not explicitly set to the query instance.

You can change this default for all the queries by setting the cache property `query.default-max-result`.

NOTE

====

If you don't explicitly set the `MaxResults` for a query instance, {brandname} limits the number of results returned by the query to `100`.

You can change the default limit by setting the `query.default-max-results` cache property.

====

Applied, thanks!

fax4ever · 2022-10-17T08:18:04Z

I applied the changes to the docs, thanks @dvagnero!

Both indexed and non-indexed

That is the way aggregations currently work, the inefficiency could be removed delegating them to Search! (see ISPN-13986)

-1 is not the infinite any more, it means apply the default, and the default is not the infinite anymore

Hybrid query, as they are, they require an unbounded max results, another reason to avoid using them

Co-authored-by: Dominika Vagnerova <dvagnero@redhat.com>

fax4ever · 2022-10-17T10:20:51Z

Applied the last changes from @dvagnero.
I think that we can merge it if the CI passes.

tristantarrant · 2022-10-17T10:55:57Z

In the previous run there were two thread leaks associated with indexing tests:

ThreadLeakChecker – org.infinispan.query.api.ManualIndexingTest
ThreadLeakChecker – org.infinispan.query.distributed.MassIndexingTest

fax4ever · 2022-10-17T12:58:28Z

In the previous run there were two thread leaks associated with indexing tests:

ThreadLeakChecker – org.infinispan.query.api.ManualIndexingTest ThreadLeakChecker – org.infinispan.query.distributed.MassIndexingTest

@tristantarrant I don't know how the current changes may affect those tests. I think that we could have the same failures on the main branch.

tristantarrant · 2022-10-17T13:04:48Z

Merged, thanks

fax4ever · 2022-10-17T13:05:02Z

Thank you!

tristantarrant added this to the 14.0.1.Final milestone Oct 11, 2022

domiborges reviewed Oct 11, 2022

View reviewed changes

fax4ever force-pushed the ISPN-14194 branch from 28cc442 to fe25d7a Compare October 13, 2022 12:19

fax4ever force-pushed the ISPN-14194 branch 2 times, most recently from 66110c0 to d702cd0 Compare October 14, 2022 07:40

fax4ever marked this pull request as ready for review October 14, 2022 08:57

tristantarrant reviewed Oct 14, 2022

View reviewed changes

tristantarrant requested changes Oct 14, 2022

View reviewed changes

fax4ever force-pushed the ISPN-14194 branch from a267010 to 338e477 Compare October 14, 2022 13:55

domiborges reviewed Oct 14, 2022

View reviewed changes

fax4ever force-pushed the ISPN-14194 branch from ada0b84 to 2a4b3e2 Compare October 17, 2022 08:15

fax4ever added 9 commits October 17, 2022 12:11

ISPN-14194 Add config property for default max result

9f7ed7d

ISPN-14194 Parse default max result query config

e3e63db

ISPN-14194 Test parsing default max result query config

848405e

ISPN-14194 Apply default max result to queries

9db5a17

ISPN-14194 Test default max result of embedded queries

1070179

Both indexed and non-indexed

ISPN-14194 Test default max result of remote queries

3be2bef

Both indexed and non-indexed

ISPN-14194 Restore the inefficiency for the aggregation

c913b15

That is the way aggregations currently work, the inefficiency could be removed delegating them to Search! (see ISPN-13986)

ISPN-14194 Align the test to the new conventions

53772c1

-1 is not the infinite any more, it means apply the default, and the default is not the infinite anymore

ISPN-14194 Restore the inefficiency for the hybrid query

66bb9fd

Hybrid query, as they are, they require an unbounded max results, another reason to avoid using them

fax4ever and others added 2 commits October 17, 2022 12:11

ISPN-14194 Generalize concept of QueryDefinition defaultMaxResults

349f959

ISPN-14194 Document query default max results

09d6bbf

Co-authored-by: Dominika Vagnerova <dvagnero@redhat.com>

fax4ever force-pushed the ISPN-14194 branch from 2a4b3e2 to 09d6bbf Compare October 17, 2022 10:19

tristantarrant merged commit bf9dd71 into infinispan:main Oct 17, 2022

fax4ever deleted the ISPN-14194 branch October 17, 2022 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISPN-14194 Add config property for default max result #10378

ISPN-14194 Add config property for default max result #10378

fax4ever commented Oct 10, 2022 •

edited

domiborges Oct 11, 2022 •

edited

wfink Oct 17, 2022

tristantarrant commented Oct 12, 2022

fax4ever commented Oct 13, 2022

fax4ever commented Oct 14, 2022

tristantarrant Oct 14, 2022

fax4ever Oct 14, 2022

tristantarrant Oct 14, 2022

tristantarrant left a comment

tristantarrant commented Oct 14, 2022

fax4ever commented Oct 14, 2022

fax4ever commented Oct 14, 2022

fax4ever commented Oct 14, 2022 •

edited

domiborges Oct 14, 2022 •

edited

tristantarrant Oct 14, 2022

fax4ever Oct 17, 2022

domiborges Oct 17, 2022

fax4ever Oct 17, 2022

domiborges Oct 14, 2022

fax4ever Oct 17, 2022

fax4ever commented Oct 17, 2022 •

edited

fax4ever commented Oct 17, 2022

tristantarrant commented Oct 17, 2022 •

edited

fax4ever commented Oct 17, 2022

tristantarrant commented Oct 17, 2022

fax4ever commented Oct 17, 2022

		The default limit of `100` is applied if `maxResults` is not explicitly set to the query instance.
		You can change this default for all the queries by setting the cache property `query.default-max-result`.

ISPN-14194 Add config property for default max result #10378

ISPN-14194 Add config property for default max result #10378

Conversation

fax4ever commented Oct 10, 2022 • edited

domiborges Oct 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tristantarrant commented Oct 12, 2022

fax4ever commented Oct 13, 2022

fax4ever commented Oct 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tristantarrant left a comment

Choose a reason for hiding this comment

tristantarrant commented Oct 14, 2022

fax4ever commented Oct 14, 2022

fax4ever commented Oct 14, 2022

fax4ever commented Oct 14, 2022 • edited

domiborges Oct 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fax4ever commented Oct 17, 2022 • edited

fax4ever commented Oct 17, 2022

tristantarrant commented Oct 17, 2022 • edited

fax4ever commented Oct 17, 2022

tristantarrant commented Oct 17, 2022

fax4ever commented Oct 17, 2022

fax4ever commented Oct 10, 2022 •

edited

domiborges Oct 11, 2022 •

edited

fax4ever commented Oct 14, 2022 •

edited

domiborges Oct 14, 2022 •

edited

fax4ever commented Oct 17, 2022 •

edited

tristantarrant commented Oct 17, 2022 •

edited