ESQL: Syntax support and operator for count all #99602

costin · 2023-09-14T22:47:36Z

Introduce support for COUNT(*) and along with a dedicated Lucene source for pushing it down instead of reading all the items one by one.

This PR doesn't handle COUNT( * ) outside Lucene (on blocks) - will take care of that (if needed) in a separate PR.
This PR takes advantage of certain stats that can be implemented at Lucene level through a query even when a filter exists (whether it is specified in the query or outside) - in this PR, count().
When just a count() aggregation is encountered (regardless of whether a filter exists or not), the Physical optimizer will convert it into a dedicated source (LuceneCountSource) which will return the intermediate aggregation state (the count and seen bool).

There are several improvements to be done in the future:

implement a similar Min and Max source to take advantage of BKD trees - this means running a sorting query (asc/desc) with limit 1
push-down count(field) - count over (filter exists query)
push down unique/stats by x as a terms query
support aggregations that can be pushed down alongside those that cannot - this means creating multiple sources and doing a union before moving forward.

Wip

dnhatn

Thanks @costin. I left some comments for the count operator. I can push these changes if you prefer.

dnhatn · 2023-09-15T04:23:03Z

.../plugin/esql/compute/src/main/java/org/elasticsearch/compute/lucene/LuceneCountOperator.java

+                // check to not go over limit
+                var count = Math.min(leafCount, remainingDocs);
+                totalHits += count;
+                remainingDocs -= count;


We need to advance the position of the scorer after we use the shortcut.

dnhatn · 2023-09-15T04:23:30Z

.../plugin/esql/compute/src/main/java/org/elasticsearch/compute/lucene/LuceneCountOperator.java

+            Page page = null;
+            if (remainingDocs <= 0 || scorer.isDone()) {
+                pagesEmitted++;
+                page = new Page(1, IntBlock.newConstantBlockWith(totalHits, 1));


This operator should emit only one page.

.../plugin/esql/compute/src/main/java/org/elasticsearch/compute/lucene/LuceneCountOperator.java

...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java

.../plugin/esql/compute/src/main/java/org/elasticsearch/compute/lucene/LuceneCountOperator.java

Use internal aggs when pushing down count

elasticsearchmachine · 2023-09-20T22:22:55Z

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

elasticsearchmachine · 2023-09-20T22:22:55Z

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

costin · 2023-09-20T22:28:48Z

Relates #99459

costin · 2023-09-21T01:36:21Z

.../plugin/esql/compute/src/main/java/org/elasticsearch/compute/lucene/LuceneCountOperator.java

+ * 1. the count as a long (0 if no doc is seen)
+ * 2. a bool flag (seen) that's always true meaning that the group (all items) always exists
+ */
+public class LuceneCountOperator extends LuceneOperator {


This class is very similar to LuceneSourceOperator - I tried combining the two but there's not much code reuse and the inner state and semantics are fairly different.

costin · 2023-09-21T01:37:21Z

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/lucene/LuceneOperator.java

+        Weight weight() {
+            return weight;
+        }
+
+        int position() {
+            return position;
+        }


Made these accessible for the count source.

costin · 2023-09-21T01:41:15Z

...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java

-    private PhysicalOperation planEsQueryNode(EsQueryExec esQuery, LocalExecutionPlannerContext context) {
-        if (esQuery.query() == null) {


The query check and initialization happens inside physicalOperatorProviders method.

costin · 2023-09-21T01:42:06Z

...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java

+        EsPhysicalOperationProviders esProvider = (EsPhysicalOperationProviders) physicalOperationProviders;
+
+        Function<SearchContext, Query> querySupplier = EsPhysicalOperationProviders.querySupplier(statsQuery.query());
+
+        Expression limitExp = statsQuery.limit();
+        int limit = limitExp != null ? (Integer) limitExp.fold() : NO_LIMIT;
+        final LuceneOperator.Factory luceneFactory = new LuceneCountOperator.Factory(
+            esProvider.searchContexts(),
+            querySupplier,
+            context.dataPartitioning(),
+            context.taskConcurrency(),
+            limit
+        );
+
+        Layout.Builder layout = new Layout.Builder();
+        layout.append(statsQuery.outputSet());
+        int instanceCount = Math.max(1, luceneFactory.taskConcurrency());
+        context.driverParallelism(new DriverParallelism(DriverParallelism.Type.DATA_PARALLELISM, instanceCount));
+        return PhysicalOperation.fromSource(luceneFactory, layout.build());


I've copied the code from esSource for now - this is candidate for future factoring.

astefan · 2023-09-21T15:08:03Z

This issue is more visible with count(*). For example

FROM employees | stats min = min(emp_no), c = count(*)
FROM employees | stats c = count(*) by gender

both fail with that NPE with the difference that these types of queries are more likely than min(123).

astefan · 2023-09-21T15:11:42Z

FROM employees | stats min = min(salary) by gender | eval x = min + 1 | stats c = count(*) by gender this one fails almost the same but with a slightly different error message:

{
    "error": {
        "root_cause": [
            {
                "type": "null_pointer_exception",
                "reason": null
            }
        ],
        "type": "null_pointer_exception",
        "reason": null,
        "suppressed": [
            {
                "type": "task_cancelled_exception",
                "reason": "parent task was cancelled [cancelled]"
            },
            {
                "type": "task_cancelled_exception",
                "reason": "parent task was cancelled [cancelled]"
            }
        ]
    },
    "status": 500
}

costin · 2023-09-22T02:55:49Z

@astefan

This PR doesn't handle COUNT( * ) outside Lucene (on blocks) - will take care of that (if needed) in a separate PR.

However from your comments I agree that this is confusing - I've pushed a PR that adds that in along with some (your) tests.
Thanks!

costin · 2023-09-22T02:56:21Z

...sql/compute/src/main/java/org/elasticsearch/compute/aggregation/CountAggregatorFunction.java

    }

    @Override
    public int intermediateBlockCount() {
        return intermediateStateDesc().size();
    }

+    private int blockIndex() {
+        return countAll ? 0 : channels.get(0);


@nik9000 not sure if this is the best way to count things in a block but it seems to be working.

costin · 2023-09-22T02:56:43Z

...ute/src/main/java/org/elasticsearch/compute/aggregation/CountGroupingAggregatorFunction.java

@@ -51,33 +57,35 @@ public int intermediateBlockCount() {

    @Override
    public AddInput prepareProcessPage(SeenGroupIds seenGroupIds, Page page) {


@nik9000 likewise here

costin · 2023-09-22T05:23:30Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats.csv-spec

@@ -508,7 +508,7 @@ from employees | stats c = count(*) by gender | sort gender;
 c:l | gender:s
 33  | F 
 57  | M
-10  | null
+#10  | null


@nik9000 There's a discrepancy between EsqlActionIT (running in Lucene) and CsvTests - the former don't have a null group for gender while the latter do hence why the same csv-spec fails for one but passes for the other.
I've commented this line to make EsqlActionIT pass but it will make CsvTests fail.
Since the count works inside the group, I wonder if null group is either discarded when querying against Lucene or if the underlying filter doesn't work properly (ignores missing values somehow).
Any ideas?

FTR, this was caused by #100109

costin · 2023-09-22T05:23:36Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats.csv-spec

@@ -517,7 +517,7 @@ from employees | stats c = count(*), min = min(emp_no) by gender | sort gender;
 c:l | min:i    | gender:s
 33  | 10002    | F 
 57  | 10001    | M
-10  | 10010    | null
+#10  | 10010    | null


costin · 2023-09-22T05:23:45Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats.csv-spec

@@ -526,5 +526,5 @@ from employees | stats min = min(salary) by gender | eval x = min + 1 | stats c
 c:l | gender:s
 1   | F 
 1   | M
-1   | null
+#1   | null


astefan

LGTM

...sql/compute/src/main/java/org/elasticsearch/compute/aggregation/CountAggregatorFunction.java

...in/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizer.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java

…count-all-bk

Introduce physical plan for representing query stats Use internal aggs when pushing down count Add support for count all outside Lucene

costin added the :Analytics/ES|QL AKA ESQL label Sep 14, 2023

costin self-assigned this Sep 14, 2023

elasticsearchmachine added the v8.11.0 label Sep 14, 2023

Syntax support and operator for count all

959254d

Wip

costin force-pushed the esql/count_all branch from 578701b to 959254d Compare September 14, 2023 22:53

Introduce physical plan for representing query stats

b745297

dnhatn reviewed Sep 15, 2023

View reviewed changes

dnhatn self-requested a review September 15, 2023 04:37

dnhatn reviewed Sep 15, 2023

View reviewed changes

.../plugin/esql/compute/src/main/java/org/elasticsearch/compute/lucene/LuceneCountOperator.java Show resolved Hide resolved

costin force-pushed the esql/count_all branch from 9467a49 to 76e56ee Compare September 18, 2023 20:41

Address feedback

8ad01fa

Use internal aggs when pushing down count

costin force-pushed the esql/count_all branch from a4f4f86 to b8b429f Compare September 20, 2023 22:22

costin marked this pull request as ready for review September 20, 2023 22:22

costin requested a review from dnhatn September 20, 2023 22:22

elasticsearchmachine added the Team:QL (Deprecated) Meta label for query languages team label Sep 20, 2023

costin force-pushed the esql/count_all branch from b8b429f to 2952bf9 Compare September 20, 2023 22:24

costin added the >non-issue label Sep 20, 2023

Merge branch 'main' into esql/count_all

f95e368

costin force-pushed the esql/count_all branch from 2952bf9 to f95e368 Compare September 20, 2023 22:43

costin added 2 commits September 20, 2023 15:52

Adjust page size

5e52dce

Fix finished()

99cdc89

costin requested review from luigidellaquila, astefan, bpintea and alex-spies September 21, 2023 01:24

costin commented Sep 21, 2023

View reviewed changes

Actually perform unit testing

40531e8

Add support for count all outside Lucene

1bc4833

costin commented Sep 22, 2023

View reviewed changes

costin added 2 commits September 21, 2023 21:19

Merge branch 'main' into esql/count_all

f08734f

Comment null to make EsqlSpecIT pass

9ae3566

costin commented Sep 22, 2023

View reviewed changes

astefan approved these changes Sep 22, 2023

View reviewed changes

Incorporate feedback

3a7731d

This was referenced Sep 23, 2023

ESQL: search-level count for a given field #99840

Closed

ESQL: Search-level stats operator #99842

Open

ESQL: NPE when aggregating on literals #99751

Closed

alex-spies mentioned this pull request Sep 25, 2023

ESQL: Improve agg verification #99827

Merged

costin added 4 commits September 29, 2023 16:46

Merge branch 'main' into esql/count_all

30f34b9

Use a different key for grouping

b56678b

Merge remote-tracking branch 'remotes/mine/esql/count_all' into esql/…

3097893

…count-all-bk

Disabling test triggering the CircuitBreaker

ce28f45

costin changed the title ~~Syntax support and operator for count all~~ ESQL: Syntax support and operator for count all Sep 30, 2023

costin merged commit f883dd9 into elastic:main Sep 30, 2023

costin deleted the esql/count_all branch September 30, 2023 18:09

costin mentioned this pull request Sep 30, 2023

ESQL: support count all / count(*) with push down count when dealing with a filter #99459

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Syntax support and operator for count all #99602

ESQL: Syntax support and operator for count all #99602

costin commented Sep 14, 2023 •

edited

Loading

dnhatn left a comment

dnhatn Sep 15, 2023

dnhatn Sep 15, 2023

elasticsearchmachine commented Sep 20, 2023

elasticsearchmachine commented Sep 20, 2023

costin commented Sep 20, 2023

costin Sep 21, 2023

costin Sep 21, 2023

costin Sep 21, 2023

costin Sep 21, 2023

astefan commented Sep 21, 2023

astefan commented Sep 21, 2023

costin commented Sep 22, 2023

costin Sep 22, 2023

costin Sep 22, 2023

costin Sep 22, 2023

costin Sep 30, 2023

costin Sep 22, 2023

costin Sep 22, 2023

astefan left a comment

		private PhysicalOperation planEsQueryNode(EsQueryExec esQuery, LocalExecutionPlannerContext context) {
		if (esQuery.query() == null) {

		@@ -51,33 +57,35 @@ public int intermediateBlockCount() {

		@Override
		public AddInput prepareProcessPage(SeenGroupIds seenGroupIds, Page page) {

ESQL: Syntax support and operator for count all #99602

ESQL: Syntax support and operator for count all #99602

Conversation

costin commented Sep 14, 2023 • edited Loading

dnhatn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Sep 20, 2023

elasticsearchmachine commented Sep 20, 2023

costin commented Sep 20, 2023

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

astefan commented Sep 21, 2023

astefan commented Sep 21, 2023

costin commented Sep 22, 2023

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

astefan left a comment

Choose a reason for hiding this comment

costin commented Sep 14, 2023 •

edited

Loading