SQL: Avoid empty last pages for GROUP BY queries when possible #84356

Luegg · 2022-02-24T17:13:40Z

Resolves #75528.

Instead of always returning an empty last page for GROUP BY queries, CompositeAggCursor will now only do so if it is not possible to tell wether there are more pages based on the composite aggregation response. This is the case in two situations:

The last page contains exactly fetch_size results. In this case, the composite aggregation return an after_key even if there are no more keys remaining (see also A way to tell whether more buckets can be fetched with after_key in composite aggregation #75573)
The query uses a bucket selector. In this case, the composite aggregation might return partial pages with less than size buckets and the buckets.size() < sizeRequested heuristic for detecting last pages does no longer work.

Hence, if any (or both) of the two conditions above applies, SQL will still return an empty last page. If neither of the conditions apply, the last page will always be non-empty.

This PR is also a weak prerequisite for addressing #84349 because it allows to immediately close PITs for aggregation queries returning only one page. As a result the performance impact of using PIT for aggregations should be minimized.

elasticsearchmachine · 2022-02-24T17:14:40Z

Hi @Luegg, I've created a changelog YAML for you.

Luegg · 2022-02-28T14:30:24Z

...lugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/CompositeAggCursor.java

    ) {

        if (log.isTraceEnabled()) {
            logSearchResponse(response, log);
        }
-        // there are some results
-        if (response.getAggregations().asList().isEmpty() == false) {


This was always true...

Luegg · 2022-02-28T14:32:32Z

...lugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/CompositeAggCursor.java


-            try {


The try/catch seems to be a leftover that could have been removed in #83833

Luegg · 2022-02-28T14:34:28Z

...lugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/CompositeAggCursor.java

-                CompositeAggRowSet rowSet = makeRowSet.get();
-                Map<String, Object> afterKey = rowSet.afterKey();
+        // retry
+        if (mightProducePartialPages && shouldRetryDueToEmptyPage(response)) {


Adding mightProducePartialPages here ensures that we do not retry for queries without bucket selectors.

elasticmachine · 2022-03-01T12:08:13Z

Pinging @elastic/es-ql (Team:QL)

astefan

LGTM

bpintea

LGTM.

bpintea · 2022-03-04T20:55:49Z

...lugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/CompositeAggCursor.java

@@ -162,71 +163,64 @@ static void handle(
        BiFunction<SearchSourceBuilder, CompositeAggRowSet, CompositeAggCursor> makeCursor,
        Runnable retry,
        ActionListener<Page> listener,
-        Schema schema
+        boolean mightProducePartialPages


Nit: couldProducePartialPage would be more suggestive to me (for both arg and method).

luigidellaquila

LGTM

costin

LGTM. A small nit, the issue description could be tweaked a bit to make it clear when the empty page is being consumed (to not pass it to the client) as oppose to when it is NOT, e.g.: when the result size is less than the fetch size AND there are no bucket selector/pipeline aggregations.

Personally I had to read the description several times to understand whether the conditions listed where for the NOT case or not (and thus if there was an AND or OR between them).

costin · 2022-03-09T17:27:41Z

...lugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/CompositeAggCursor.java

-    private static void updateCompositeAfterKey(SearchResponse r, SearchSourceBuilder search) {
-        CompositeAggregation composite = getComposite(r);
+    static boolean mightProducePartialPages(CompositeAggregationBuilder aggregation) {
+        return aggregation.getPipelineAggregations().stream().anyMatch(a -> a instanceof BucketSelectorPipelineAggregationBuilder);


I don't think we're using any other type of pipeline aggregation so simply returning if the size > 0 should be enough.
Also not a fun of using the streams api due to their cost - the above can become:

for (var a : aggregation.getPipelineAggregrations()) { return a instanceof BucketSelectorPipelineAggregationBuilder }

costin · 2022-03-09T17:30:26Z

...k/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/SearchHitCursor.java

                listener::onFailure
            )
        );
    }

-    private Supplier<SearchHitRowSet> makeRowSet(int sizeRequested, SearchResponse response) {
-        return () -> new SearchHitRowSet(extractors, mask, sizeRequested, limit, response);
+    private Supplier<SearchHitRowSet> makeRowSet(SearchResponse response) {


elasticsearchmachine · 2022-03-14T11:31:38Z

💔 Backport failed

Status	Branch	Result
❌	8.1	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 84356

Luegg · 2022-03-14T12:42:14Z

Not backporting to 8.1.1 because of changeset overlapping with #83381

Resolves #84349 This PR has a small overlap with #84356 but can be merged independently.

…ic#84356) Resolves elastic#75528. Instead of always returning an empty last page for `GROUP BY` queries, `CompositeAggCursor` will now only do so if it is not possible to tell wether there are more pages based on the composite aggregation response. This is the case in two situations: * The last page contains exactly `fetch_size` results. In this case, the composite aggregation return an `after_key` even if there are no more keys remaining (see also elastic#75573) * The query uses a bucket selector. In this case, the composite aggregation might return partial pages with less than `size` buckets and the `buckets.size() < sizeRequested` heuristic for detecting last pages does no longer work. Hence, if any (or both) of the two conditions above applies, SQL will still return an empty last page. If neither of the conditions apply, the last page will always be non-empty. This PR is also a weak prerequisite for addressing elastic#84349 because it allows to immediately close PITs for aggregation queries returning only one page. As a result the performance impact of using PIT for aggregations should be minimized.

Resolves elastic#84349 This PR has a small overlap with elastic#84356 but can be merged independently.

…ic#84356) Resolves elastic#75528. Instead of always returning an empty last page for `GROUP BY` queries, `CompositeAggCursor` will now only do so if it is not possible to tell wether there are more pages based on the composite aggregation response. This is the case in two situations: * The last page contains exactly `fetch_size` results. In this case, the composite aggregation return an `after_key` even if there are no more keys remaining (see also elastic#75573) * The query uses a bucket selector. In this case, the composite aggregation might return partial pages with less than `size` buckets and the `buckets.size() < sizeRequested` heuristic for detecting last pages does no longer work. Hence, if any (or both) of the two conditions above applies, SQL will still return an empty last page. If neither of the conditions apply, the last page will always be non-empty. This PR is also a weak prerequisite for addressing elastic#84349 because it allows to immediately close PITs for aggregation queries returning only one page. As a result the performance impact of using PIT for aggregations should be minimized.

Resolves elastic#84349 This PR has a small overlap with elastic#84356 but can be merged independently.

resolves #85520 The failure was caused by the assumption that groupBy queries always return a scroll cursor that has been fixed in #84356. Because we will run into the same problem again with 8.2.1 this fix also needs to be backported to 8.2.

resolves elastic#85520 The failure was caused by the assumption that groupBy queries always return a scroll cursor that has been fixed in elastic#84356. Because we will run into the same problem again with 8.2.1 this fix also needs to be backported to 8.2.

resolves #85520 The failure was caused by the assumption that groupBy queries always return a scroll cursor that has been fixed in #84356. Because we will run into the same problem again with 8.2.1 this fix also needs to be backported to 8.2.

Luegg added :Analytics/SQL SQL querying >bug labels Feb 24, 2022

elasticsearchmachine added the v8.2.0 label Feb 24, 2022

Luegg force-pushed the fix/reduceEmptyLastPages branch 2 times, most recently from 6a585a1 to 7818f65 Compare February 28, 2022 13:11

SQL: Avoid empty last pages for GROUP BY queries when possible

a5854ad

Luegg force-pushed the fix/reduceEmptyLastPages branch from 7818f65 to a5854ad Compare February 28, 2022 13:52

Update docs/changelog/84356.yaml

73c3d9c

Luegg commented Feb 28, 2022

View reviewed changes

Luegg requested review from costin, astefan, luigidellaquila and bpintea March 1, 2022 12:08

Luegg marked this pull request as ready for review March 1, 2022 12:08

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Mar 1, 2022

Luegg mentioned this pull request Mar 3, 2022

SQL: PIT for GROUP BY and PIVOT queries #84605

Merged

astefan approved these changes Mar 3, 2022

View reviewed changes

bpintea approved these changes Mar 4, 2022

View reviewed changes

luigidellaquila approved these changes Mar 8, 2022

View reviewed changes

costin approved these changes Mar 9, 2022

View reviewed changes

Luegg added 2 commits March 14, 2022 10:44

Merge origin/main into fix/reduceEmptyLastPages

d1b34a1

address comments

c587609

Luegg force-pushed the fix/reduceEmptyLastPages branch from 2f11cc3 to c587609 Compare March 14, 2022 10:28

Luegg added v8.1.1 and removed v8.1.1 labels Mar 14, 2022

Luegg added auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) auto-backport-and-merge Automatically create backport pull requests and merge when ready labels Mar 14, 2022

elasticsearchmachine merged commit 906dda0 into elastic:master Mar 14, 2022

Luegg deleted the fix/reduceEmptyLastPages branch March 14, 2022 11:30

Luegg removed the v8.1.1 label Mar 14, 2022

elasticsearchmachine pushed a commit that referenced this pull request Mar 14, 2022

SQL: PIT for GROUP BY and PIVOT queries (#84605)

c4253ad

Resolves #84349 This PR has a small overlap with #84356 but can be merged independently.

javanna pushed a commit to javanna/elasticsearch that referenced this pull request Mar 16, 2022

SQL: PIT for GROUP BY and PIVOT queries (elastic#84605)

f997aac

Resolves elastic#84349 This PR has a small overlap with elastic#84356 but can be merged independently.

CohenIdo pushed a commit to CohenIdo/elasticsearch that referenced this pull request Mar 24, 2022

SQL: PIT for GROUP BY and PIVOT queries (elastic#84605)

8d9ebfa

Resolves elastic#84349 This PR has a small overlap with elastic#84356 but can be merged independently.

Luegg mentioned this pull request Mar 31, 2022

Fix SQLCompatIT.testCursorFromOldNodeFailsOnNewNode #85531

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Avoid empty last pages for GROUP BY queries when possible #84356

SQL: Avoid empty last pages for GROUP BY queries when possible #84356

Luegg commented Feb 24, 2022 •

edited

elasticsearchmachine commented Feb 24, 2022

Luegg Feb 28, 2022

Luegg Feb 28, 2022

Luegg Feb 28, 2022

elasticmachine commented Mar 1, 2022

astefan left a comment

bpintea left a comment

bpintea Mar 4, 2022

luigidellaquila left a comment

costin left a comment

costin Mar 9, 2022

costin Mar 9, 2022

elasticsearchmachine commented Mar 14, 2022

Luegg commented Mar 14, 2022

SQL: Avoid empty last pages for GROUP BY queries when possible #84356

SQL: Avoid empty last pages for GROUP BY queries when possible #84356

Conversation

Luegg commented Feb 24, 2022 • edited

elasticsearchmachine commented Feb 24, 2022

Luegg Feb 28, 2022

Choose a reason for hiding this comment

Luegg Feb 28, 2022

Choose a reason for hiding this comment

Luegg Feb 28, 2022

Choose a reason for hiding this comment

elasticmachine commented Mar 1, 2022

astefan left a comment

Choose a reason for hiding this comment

bpintea left a comment

Choose a reason for hiding this comment

bpintea Mar 4, 2022

Choose a reason for hiding this comment

luigidellaquila left a comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

costin Mar 9, 2022

Choose a reason for hiding this comment

costin Mar 9, 2022

Choose a reason for hiding this comment

elasticsearchmachine commented Mar 14, 2022

💔 Backport failed

Luegg commented Mar 14, 2022

Luegg commented Feb 24, 2022 •

edited