Skip to content

Commit 90173b5

Browse files
[ES|QL] Correct line and column numbers of missing named parameters (#120852) (#121451)
* correct line and column numbers of missing named parameters
1 parent 6b15f92 commit 90173b5

File tree

4 files changed

+71
-11
lines changed

4 files changed

+71
-11
lines changed

docs/changelog/120852.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 120852
2+
summary: Correct line and column numbers of missing named parameters
3+
area: ES|QL
4+
type: bug
5+
issues: []

x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -774,6 +774,33 @@ public void testNamedParamsForIdentifierAndIdentifierPatterns() throws IOExcepti
774774
}
775775
}
776776

777+
public void testErrorMessageForMissingParams() throws IOException {
778+
ResponseException re = expectThrows(
779+
ResponseException.class,
780+
() -> runEsql(requestObjectBuilder().query("from idx | where x == ?n1").params("[]"))
781+
);
782+
assertThat(
783+
EntityUtils.toString(re.getResponse().getEntity()).replaceAll("\\\\\n\s+\\\\", ""),
784+
containsString("line 1:23: Unknown query parameter [n1]")
785+
);
786+
787+
re = expectThrows(
788+
ResponseException.class,
789+
() -> runEsql(requestObjectBuilder().query("from idx | where x == ?n1 and y == ?n2").params("[{\"n\" : \"v\"}]"))
790+
);
791+
assertThat(EntityUtils.toString(re.getResponse().getEntity()).replaceAll("\\\\\n\s+\\\\", ""), containsString("""
792+
line 1:23: Unknown query parameter [n1], did you mean [n]?; line 1:36: Unknown query parameter [n2], did you mean [n]?"""));
793+
794+
re = expectThrows(
795+
ResponseException.class,
796+
() -> runEsql(requestObjectBuilder().query("from idx | where x == ?n1 and y == ?n2").params("[{\"n1\" : \"v1\"}]"))
797+
);
798+
assertThat(
799+
EntityUtils.toString(re.getResponse().getEntity()).replaceAll("\\\\\n\s+\\\\", ""),
800+
containsString("line 1:36: Unknown query parameter [n2], did you mean [n1]")
801+
);
802+
}
803+
777804
public void testErrorMessageForLiteralDateMathOverflow() throws IOException {
778805
List<String> dateMathOverflowExpressions = List.of(
779806
"2147483647 day + 1 day",

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -110,17 +110,7 @@ protected LogicalPlan plan(ParseTree ctx) {
110110
if (errors.hasNext() == false) {
111111
return p;
112112
} else {
113-
StringBuilder message = new StringBuilder();
114-
int i = 0;
115-
116-
while (errors.hasNext()) {
117-
if (i > 0) {
118-
message.append("; ");
119-
}
120-
message.append(errors.next().getMessage());
121-
i++;
122-
}
123-
throw new ParsingException(message.toString());
113+
throw ParsingException.combineParsingExceptions(errors);
124114
}
125115
}
126116

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ParsingException.java

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
import org.elasticsearch.xpack.esql.EsqlClientException;
1010
import org.elasticsearch.xpack.esql.core.tree.Source;
1111

12+
import java.util.Iterator;
13+
1214
import static org.elasticsearch.common.logging.LoggerMessageFormat.format;
1315

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

26+
/**
27+
* To be used only if the exception cannot be associated with a specific position in the query.
28+
* Error message will start with {@code line -1:-1:} instead of using specific location.
29+
*/
2430
public ParsingException(String message, Object... args) {
2531
this(Source.EMPTY, message, args);
2632
}
@@ -37,6 +43,38 @@ public ParsingException(Exception cause, Source source, String message, Object..
3743
this.charPositionInLine = source.source().getColumnNumber();
3844
}
3945

46+
private ParsingException(int line, int charPositionInLine, String message, Object... args) {
47+
super(message, args);
48+
this.line = line;
49+
this.charPositionInLine = charPositionInLine;
50+
}
51+
52+
/**
53+
* Combine multiple {@code ParsingException} into one, this is used by {@code LogicalPlanBuilder} to
54+
* consolidate multiple named parameters related {@code ParsingException}.
55+
*/
56+
public static ParsingException combineParsingExceptions(Iterator<ParsingException> parsingExceptions) {
57+
StringBuilder message = new StringBuilder();
58+
int i = 0;
59+
int line = -1;
60+
int charPositionInLine = -1;
61+
62+
while (parsingExceptions.hasNext()) {
63+
ParsingException e = parsingExceptions.next();
64+
if (i > 0) {
65+
message.append("; ");
66+
message.append(e.getMessage());
67+
} else {
68+
// line and column numbers are the associated with the first error
69+
line = e.getLineNumber();
70+
charPositionInLine = e.getColumnNumber();
71+
message.append(e.getErrorMessage());
72+
}
73+
i++;
74+
}
75+
return new ParsingException(line, charPositionInLine, message.toString());
76+
}
77+
4078
public int getLineNumber() {
4179
return line;
4280
}

0 commit comments

Comments
 (0)