Skip to content

Conversation

@bpintea
Copy link
Contributor

@bpintea bpintea commented Sep 5, 2025

This adds the guards to reject queries that INLINESTATS cannot currently execute:

  • SORT with no LIMIT before INLINESTATS
  • CATEGORIZE with INLINESTATS.

Closes #124725

@bpintea bpintea requested a review from astefan September 5, 2025 16:50
@bpintea bpintea removed the WIP label Sep 5, 2025
@bpintea bpintea marked this pull request as ready for review September 5, 2025 16:52
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 5, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM

* failures.add(
* fail(
* inlineJoin,
* "unbounded sort [{}] not supported before inlinejoin [{}], move the sort after the inlinejoin",
Copy link
Contributor

Choose a reason for hiding this comment

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

This being a user facing error message where there is no notion of "inlinejoin" we might want to adapt it to the command name itself (inlinestats / inline 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.

Ah, right, thanks.

orderBy -> failures.add(
fail(
inlineJoin,
"inlinestats [{}] cannot yet have an unbounded sort [{}] before it : either move the sort after it,"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: uppercase inlinestats (like the message from Categorize).

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've initially kept it in sync with the existing message in OrderBy about general unbounded sorts, which didn't use capitals. But I've updated that now too, to use capitals for commands.

@bpintea bpintea added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 9, 2025
@elasticsearchmachine elasticsearchmachine merged commit e3d7ba3 into elastic:main Sep 9, 2025
34 checks passed
@bpintea bpintea deleted the enh/inlinestats_fail_messages branch September 9, 2025 10:54
rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Sep 9, 2025
…astic#134201)

This adds the guards to reject queries that INLINESTATS cannot currently
execute: - SORT with no LIMIT before INLINESTATS - CATEGORIZE with
INLINESTATS.

Closes elastic#124725
Kubik42 pushed a commit to Kubik42/elasticsearch that referenced this pull request Sep 9, 2025
…astic#134201)

This adds the guards to reject queries that INLINESTATS cannot currently
execute: - SORT with no LIMIT before INLINESTATS - CATEGORIZE with
INLINESTATS.

Closes elastic#124725
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ESQL: Ensure we fail nicely for invalid INLINESTATS queries

3 participants