[ES|QL] Correct line and column numbers of missing named parameters

[fang-xing-esql]

Resolves: #120778

The parsing errors related to missing named parameters do not have a proper position associated with them, when the errors are collected together at the end of parser. This PR assign the first error's position as the position of the consolidated errors.

Before: the location of the error message is line -1:-1:

{
  "query": "from sample_data | where client.ip == ?ip and @timestamp > ?time",
  "params" : []
}
'
{
  "error" : {
    "root_cause" : [
      {
        "type" : "parsing_exception",
        "reason" : "line -1:-1: line 1:39: Unknown query parameter [ip]; line 1:60: Unknown query parameter [time]"
      }
    ],
    "type" : "parsing_exception",
    "reason" : "line -1:-1: line 1:39: Unknown query parameter [ip]; line 1:60: Unknown query parameter [time]"
  },
  "status" : 400
}

After: the location of the first error is returned as the location of the whole message.

{
  "query": "from sample_data | where client.ip == ?ip and @timestamp > ?time",
  "params" : []
}
'
{
  "error" : {
    "root_cause" : [
      {
        "type" : "parsing_exception",
        "reason" : "line 1:39: Unknown query parameter [ip]; line 1:60: Unknown query parameter [time]"
      }
    ],
    "type" : "parsing_exception",
    "reason" : "line 1:39: Unknown query parameter [ip]; line 1:60: Unknown query parameter [time]"
  },
  "status" : 400
}

[elasticsearchmachine]

Hi @fang-xing-esql, I've created a changelog YAML for you.

[elasticsearchmachine]

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

[elasticsearchmachine]

Pinging @elastic/kibana-esql (ES|QL-ui)

[stratoula]

Thanx Fang 🙌

[alex-spies]

LGTM, thanks @fang-xing-esql ! Only minor comments from me :)

[alex-spies]

Broken changelog reference :/

[alex-spies]

optional: While we're at it, could we maybe add a short warning javadoc to the constructor that we no longer use for this, the one that doesn't take a source or line/char numbers? I think it'd help to point out that the error message will start with line -1:-1, that may not be obvious without looking more closely. Something like

    /**
     * To be used only if the exception cannot be associated with a specific position in the query. Error message will start with
     * {@code line -1:-1:} instead of using specific line/character numbers. 
     */

[fang-xing-esql]

Good point, will add a comment there.

[alex-spies]

nit: the changes here essentially just implement squashing multiple ParsingExceptions into a single one. I think it'd be a tad nicer if that logic was encapsulated in ParsingException, maybe as a method ParsingException.combineWith(ParsingException... others) or so.

[fang-xing-esql]

It makes sense, I was thinking about the same thing, I'll move these into ParsingException.

[ivancea]

LGTM! A comment with a suggestion. Not sure how big the change is tho

[ivancea]

Nit: Would it be worth it to add a CompoundParsingException class doing this? Maybe a subclass, to avoid any potential code breaking (I see some tests using getLineNumber, we could refactor those).
Some pros of this:

• The getErrorMessage returns a weird message in this PR, a First error; line 1:2: Second error; line 4:5: Third error, missing the line on the first one only
• To wrap this logic inside a new CompoundParsingException(exceptions)
• No need for the new constructor without source

Very nit really, feel free to ignore! Especially if you think it could add some extra complexity somewhere

[ivancea]

Adding to this, I see some uses of instanceof ParsingException just to do a .getErrorMessage() instead of getMessage(). I would check if those would still work "as expected" with this change

[fang-xing-esql]

Here we indeed return a combined ParsingException that represents a collection of ParsingExceptions related to named parameters.

It is actually not a bad idea to have a CompoundParsingException I think, the only thing that I can think of is that the downstream components may need to deal with them differently, if we throw a ParsingException, they can locate it in the query text directly, however if we throw a CompoundParsingException, they may need to iterate through each ParsingException that belongs to the CompoundParsingException to extract the locations, this could be a better way to present all parsing errors that are related to the named parameters, it requires changes in kibana potentially. For now, I'll combine them into one ParsingException, CompoundParsingException is a potential future enhancement if we need a better way to present these errors.

getErrorMessage() does not include the location of the query text, I double checked, there are a couple of places that call it directly, they don't require the location information.

[fang-xing-esql]

@elasticmachine update branch

[fang-xing-esql]

@elasticmachine update branch

[fang-xing-esql]

@elasticmachine update branch

[fang-xing-esql]

@elasticmachine update branch

[elasticsearchmachine]

💚 Backport successful

Status	Branch	Result
✅	9.0
✅	8.18
✅	8.x