Skip to content

ES|QL: manage INLINE STATS count(*) on result sets with no columns #137017

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Oct 24, 2025
Merged

ES|QL: manage INLINE STATS count(*) on result sets with no columns #137017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b024558
ES|QL: manage INLINE STATS count(*) on result sets with no columns
luigidellaquila Oct 23, 2025
6526e5a
Update docs/changelog/137017.yaml
luigidellaquila Oct 23, 2025
7e4a92d
Test with ROW
luigidellaquila Oct 23, 2025
5345455
Simplify + comments
luigidellaquila Oct 23, 2025
b66682d
Merge branch 'main' into esql/fix_inline_stats_no_columns
luigidellaquila Oct 23, 2025
b2d945e
Merge branch 'main' into esql/fix_inline_stats_no_columns
luigidellaquila Oct 23, 2025
720bc36
Merge remote-tracking branch 'luigidellaquila/esql/fix_inline_stats_n…
luigidellaquila Oct 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/137017.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 137017
summary: Manage INLINE STATS count(*) on result sets with no columns
area: ES|QL
type: bug
issues: []
39 changes: 29 additions & 10 deletions .../compute/src/main/java/org/elasticsearch/compute/aggregation/CountAggregatorFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,23 +78,42 @@ public int intermediateBlockCount() {
}

private int blockIndex() {
return countAll ? 0 : channels.get(0);
// In case of countAll, block index is irrelevant.
// Page.positionCount should be used instead,
// because the page could have zero blocks
// (drop all columns scenario)
return countAll ? -1 : channels.get(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this was wrong: countAll does not rely on blocks, it only relies on page.positionCount

Copy link

@cimequinox cimequinox Oct 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intent here to return an illegal value when countAll is true given that addRawInput does not call blockIndex in that case? E.g. in case a future modification leads to calling page.getBlock(-1) at line 100 ?

Also curious if the intent is to trigger the assert in addIntermediateInput in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the assert should better protect us now, in case of empty pages.

page.getBlock(-1) will never happen with current implementation, and in case of modifications it will throw a clear error (IMHO better than silently returning a wrong value, because we are calculating results based on the wrong block)

}

@Override
public void addRawInput(Page page, BooleanVector mask) {
Block block = page.getBlock(blockIndex());
LongState state = this.state;
int count;
if (mask.isConstant()) {
if (mask.getBoolean(0) == false) {
return;
if (countAll) {
// this will work also when the page has no blocks
if (mask.isConstant() && mask.getBoolean(0)) {
state.longValue(state.longValue() + page.getPositionCount());
} else {
int count = 0;
for (int i = 0; i < mask.getPositionCount(); i++) {
if (mask.getBoolean(i)) {
count++;
}
}
state.longValue(state.longValue() + count);
}
count = countAll ? block.getPositionCount() : block.getTotalValueCount();
} else {
count = countMasked(block, mask);
Block block = page.getBlock(blockIndex());
LongState state = this.state;
int count;
if (mask.isConstant()) {
if (mask.getBoolean(0) == false) {
return;
}
count = block.getTotalValueCount();
} else {
count = countMasked(block, mask);
}
state.longValue(state.longValue() + count);
}
state.longValue(state.longValue() + count);
}

private int countMasked(Block block, BooleanVector mask) {
Expand Down
29 changes: 29 additions & 0 deletions x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4362,3 +4362,32 @@ from employees
c:long
1
;


inlineStatsKeepDropCountStar
required_capability: inline_stats_with_no_columns

from employees
| keep emp_no
| drop emp_no
| inline stats c = count(*)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix doesn't seem to be inline stats specific. If so (please, correct me if I'm wrong), there should be use cases where a regular stats should reveal this error, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original query had a STATS as last command.
Unfortunately it doesn't run anymore, because of the limitations introduced by #136981, and without the LIMIT the query works just fine.
(Disabling that limitation, and with this fix, the original query doesn't fail anymore btw)

I suspect that the problem could still happen with STATS, but I couldn't find a way to reproduce it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect that the problem could still happen with STATS

I mean, it could have happened, but this fix should also cover normal STATS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to debug the two queries

ROW a = 12 | DROP a | STATS count(*)

ROW a = 12 | DROP a | INLINE STATS b = count(*)

The interesting bit is that with INLINE STATS the page in the aggregator has no blocks, while with STATS it still has the original block for a.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am looking at this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've looked at this and your point @luigidellaquila about aggregator having a different behavior for stats vs. inline stats showed that what we do with inline stats has a "manual" step that I did question in code several times so far: marking a plan as optimized without pushing it through another round of optimizations, only because the parts it's made of were already pushed through the logical optimizer.

What happens is that with inline stats we get to execute a plan like this one below where the EsqlProject with empty projection list as a ProjectOperator on the execution layer gets to create a Page with no blocks (because there is nothing to project)

Aggregate[[],[COUNT(*[KEYWORD],true[BOOLEAN]) AS count(*)#11]]
\_EsqlProject[[]]
  \_LocalRelation[[a{r}#9],org.elasticsearch.xpack.esql.plan.logical.local.CopyingLocalSupplier@7cc]

While with regular stats (the same aggregation as above), the optimizer "merges" the two plans:

Rule logical.CombineProjections applied with change
Limit[1000[INTEGER],false,false]                                  = Limit[1000[INTEGER],false,false]
\_Aggregate[[],[COUNT(*[KEYWORD],true[BOOLEAN]) AS count(*)#117]] = \_Aggregate[[],[COUNT(*[KEYWORD],true[BOOLEAN]) AS count(*)#117]]
  \_EsqlProject[[]]                                               !   \_Row[[12[INTEGER] AS a#115]]
    \_Row[[12[INTEGER] AS a#115]]                                 !

And there is no ProjectOperator creating an empty Page for which AggregateOperator not to know what to do with it.

I am not saying the fix in this PR is not necessary @luigidellaquila (you know better than me the Page with no rows story), but the fact that the code reached a point with inline stats that it wouldn't reach with regular stats made me wonder. I have opened a Draft PR to explore an idea that I didn't explore so far because there were no concrete reasons to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the details and for the in-depth analysis @astefan!
As long as we have zero-blocks pages, I think it makes sense to have this PR merged, but I agree with you that it would be better to be consistent between STATS and INLINE STATS.

| limit 3
;

c:long
100
100
100
;

rowInlineStatsKeepDropCountStar
required_capability: inline_stats_with_no_columns

row a = 1
| drop a
| inline stats c = count(*)
;

c:long
1
;
6 changes: 6 additions & 0 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1563,11 +1563,17 @@ public enum Cap {
* Temporarily forbid the use of an explicit or implicit LIMIT before INLINE STATS.
*/
FORBID_LIMIT_BEFORE_INLINE_STATS(INLINE_STATS.enabled),

/**
* Support for the TRANGE function
*/
FN_TRANGE,

/**
* https://github.com/elastic/elasticsearch/issues/136851
*/
INLINE_STATS_WITH_NO_COLUMNS(INLINE_STATS.enabled),

// Last capability should still have a comma for fewer merge conflicts when adding new ones :)
// This comment prevents the semicolon from being on the previous capability when Spotless formats the file.
;
Expand Down