Skip to content
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.

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

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

Merged
Merged
Show file tree
Hide file tree
Changes from all 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 @@ -108,17 +108,7 @@ protected LogicalPlan plan(ParseTree ctx) {
if (errors.hasNext() == false) {
return p;
} else {
StringBuilder message = new StringBuilder();
int i = 0;

while (errors.hasNext()) {
if (i > 0) {
message.append("; ");
}
message.append(errors.next().getMessage());
i++;
}
throw new ParsingException(message.toString());
throw ParsingException.combineParsingExceptions(errors);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import org.elasticsearch.xpack.esql.EsqlClientException;
import org.elasticsearch.xpack.esql.core.tree.Source;

import java.util.Iterator;

import static org.elasticsearch.common.logging.LoggerMessageFormat.format;

public class ParsingException extends EsqlClientException {
Expand All @@ -21,6 +23,10 @@ public ParsingException(String message, Exception cause, int line, int charPosit
this.charPositionInLine = charPositionInLine + 1;
}

/**
* 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 location.
*/
public ParsingException(String message, Object... args) {
this(Source.EMPTY, message, args);
}
Expand All @@ -37,6 +43,38 @@ public ParsingException(Exception cause, Source source, String message, Object..
this.charPositionInLine = source.source().getColumnNumber();
}

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

/**
* Combine multiple {@code ParsingException} into one, this is used by {@code LogicalPlanBuilder} to
* consolidate multiple named parameters related {@code ParsingException}.
*/
public static ParsingException combineParsingExceptions(Iterator<ParsingException> parsingExceptions) {
StringBuilder message = new StringBuilder();
int i = 0;
int line = -1;
int charPositionInLine = -1;

while (parsingExceptions.hasNext()) {
ParsingException e = parsingExceptions.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());
}
i++;
}
return new ParsingException(line, charPositionInLine, message.toString());
}

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