Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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/120852.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 120852
summary: Correct line and column numbers of missing named parameters
area: ES|QL
type: bug
issues: []
Copy link
Contributor

Choose a reason for hiding this comment

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

Broken changelog reference :/

Original file line number Diff line number Diff line change
Expand Up @@ -773,6 +773,33 @@ public void testNamedParamsForIdentifierAndIdentifierPatterns() throws IOExcepti
}
}

public void testErrorMessageForMissingParams() throws IOException {
ResponseException re = expectThrows(
ResponseException.class,
() -> runEsql(requestObjectBuilder().query("from idx | where x == ?n1").params("[]"))
);
assertThat(
EntityUtils.toString(re.getResponse().getEntity()).replaceAll("\\\\\n\s+\\\\", ""),
containsString("line 1:23: Unknown query parameter [n1]")
);

re = expectThrows(
ResponseException.class,
() -> runEsql(requestObjectBuilder().query("from idx | where x == ?n1 and y == ?n2").params("[{\"n\" : \"v\"}]"))
);
assertThat(EntityUtils.toString(re.getResponse().getEntity()).replaceAll("\\\\\n\s+\\\\", ""), containsString("""
line 1:23: Unknown query parameter [n1], did you mean [n]?; line 1:36: Unknown query parameter [n2], did you mean [n]?"""));

re = expectThrows(
ResponseException.class,
() -> runEsql(requestObjectBuilder().query("from idx | where x == ?n1 and y == ?n2").params("[{\"n1\" : \"v1\"}]"))
);
assertThat(
EntityUtils.toString(re.getResponse().getEntity()).replaceAll("\\\\\n\s+\\\\", ""),
containsString("line 1:36: Unknown query parameter [n2], did you mean [n1]")
);
}

public void testErrorMessageForLiteralDateMathOverflow() throws IOException {
List<String> dateMathOverflowExpressions = List.of(
"2147483647 day + 1 day",
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
Expand Up @@ -110,15 +110,23 @@ protected LogicalPlan plan(ParseTree ctx) {
} else {
StringBuilder message = new StringBuilder();
int i = 0;
int line = -1;
int charPositionInLine = -1;

while (errors.hasNext()) {
ParsingException e = errors.next();
if (i > 0) {
message.append("; ");
message.append(e.getMessage());
} else {
// line and column numbers are the associated with the first error
line = e.getLineNumber();
charPositionInLine = e.getColumnNumber();
message.append(e.getErrorMessage());
}
message.append(errors.next().getMessage());
i++;
}
throw new ParsingException(message.toString());
throw new ParsingException(line, charPositionInLine, message.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}
}

Expand Down
Copy link
Contributor

@alex-spies alex-spies Jan 30, 2025

Choose a reason for hiding this comment

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

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. 
     */

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, will add a comment there.

Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ public ParsingException(Exception cause, Source source, String message, Object..
this.charPositionInLine = source.source().getColumnNumber();
}

public ParsingException(int line, int charPositionInLine, String message, Object... args) {
super(message, args);
this.line = line;
this.charPositionInLine = charPositionInLine;
}

public int getLineNumber() {
return line;
}
Expand Down